[5.1] Make sure unguarded() does not change unguarded state on exception

[jeroennoten]

This makes sure that when an exception is thrown in the callback passed to Model::unguarded(), the Model is still guarded afterwards. It uses the try .. finally construct. I also added a unit test.

A use case when this can happen is in FactoryBuilder::makeInstance, which throws an exception when it cannot find the model factory. In tests, this can result in strange errors in other tests, because all models are unguarded afterwards (also in other test cases) when this exception is thrown. So then tests will fail when they shouldn't. This PR prevents this kind of situations.

[taylorotwell]

Could this now be undefined if an exception is thrown and $result is never set?

[GrahamCampbell]

Yeh, this code is broken atm.

[GrahamCampbell]

Change it to:

try {
    return $callback();
} finally {
    static::reguard();
}

[jeroennoten]

I think it does not matter. When the callback throws an exception, this method throws that exception and thus does not return anything. A method either returns or throws an exception. Not both. Or am I wrong now?

[GrahamCampbell]

that is true, but writing the return inside the try like that is definitely preferred please

[jeroennoten]

You're right, that looks indeed better. I will change it a.s.a.p.

[jeroennoten]

Updated as proposed by @GrahamCampbell: moved the return statement into the try block.

[GrahamCampbell]

👍

[JosephSilber]

I don't like returning before a finally. Messes with my brain.

But that's just me.

[jeroennoten]

Yeah.. It's subjective.