Allow for creating an Auditable model even if application is not spun up

[cosmastech]

As our team focuses on creating more testable components, we prefer unit tests over feature tests. Even better for those unit tests if they don't require the Laravel framework to be booted at all.

As is the case for most Laravel applications, Models are everywhere in our codebase. Sometimes we only want to test very limited things about a model behavior, or a function that takes in a model, but only needs a property or two from it. If we try to write a unit test (extending from PHPUnit\Framework\TestCase), we cannot even call new User(['id' => 1]) because as Auditable boots, it attempts to access the App facade (which won't be set, and will raise an exception).

For instance

Consider the following example

namespace App\Models;

use OwenIt\Auditing\Contracts\Auditable;
use Illuminate\Database\Eloquent\Model;

class User extends Model implements Auditable
{
    use \OwenIt\Auditing\Auditable;

    protected $fillable = ['user_type'];

    public function isAdmin(): bool
    {
        return $this->user_type === 'admin';
    }
}

and a unit test

class UserTest extends \PHPUnit\Framework\TestCase
{
    public function test_adminUserType_returnsIsAdminTrue(): void
    {
        $user = new User(['user_type' => 'admin']);
        $this->assertTrue($user->isAdmin());
    }
}

In the current version of Auditable, this will fail on the first line of the test with a RuntimeException.

The change implemented is to just swallow up this particular exception and not to attempt to add the observer.

Other approaches

Trigger a notice

Instead of swallowing the exception, we could trigger_error(), however, then there's a "notice" attached to the test.

Leverage Auditable::isAuditingDisabled()

I tried adding this check into Auditable::isAuditingEnabled(), but it broke tests, indicating a breaking change. Also, it's possible that a model is instantiated when auditing is temporarily disabled, but then when saving or changing the model later in the request, we do want auditing enabled.

[willpower232]

Personally, I'm confused about why you wouldn't always test in the context of the laravel application since if the code in your project is being run without laravel, that sounds like you don't really need laravel 😅 but hey, you do you.

IMHO the only change I would suggest is matching the entire exception message rather than using stripos. According to github, the exception message hasn't changed for 9 years so you would hope it would be an trustworthy string match.

[cosmastech]

IMHO the only change I would suggest is matching the entire exception message rather than using stripos. According to github, the exception message hasn't changed for 9 years so you would hope it would be an trustworthy string match.

Thanks for checking that! I had the thought to check that, but was being lazy. 😝

I have made the change.

Personally, I'm confused about why you wouldn't always test in the context of the laravel application since if the code in your project is being run without laravel, that sounds like you don't really need laravel 😅 but hey, you do you.

It's mainly to avoid the setup/teardown required for Laravel's TestCase. While there's a lot convenience included with that, it's not for free. Each test case has to spin through a lot of extra code by creating, booting, and then destroying the application for something that isn't truly needed by the system under test.

The benefit to austerity in test classes is faster feedback. For a single test, this feedback is probably close to imperceptible (talking a change from 0.02 => 0.01 seconds). But for the test suite as a whole, which is run by every member of the team locally multiple times per day, plus many, many times per day in our CI/CD pipelines, this adds up as the test suite grows.

In general, I hope to write testable components that don't require Models at all, but it's a work in progress on that front. There's a lot of philosophical reasoning that I don't assume every person using Laravel would subscribe to (or even benefit from). Just hoping that this change's risk is low impact enough that it can get added to the package.

[willpower232]

You did also lint the rest of the file so if you could undo that, that would be swell 😅

[valerio-bozzolan]

P.S. it would be nice to propose a patch in Laravel framework to create a dedicated class that extends RuntimeException to throw that A facade root has not been set. so we can catch that specific class, instead of checking the message string, that is really something a bit esoteric.

https://github.com/laravel/framework/blob/11.x/src/Illuminate/Support/Facades/Facade.php#L358

[erikn69]

@willpower232 hi,
It doesn't seem correct to check the message (even though it hasn't changed since a lot), how about the approach of checking if the app facade already exists?

[cosmastech]

@willpower232 hi, It doesn't seem correct to check the message (even though it hasn't changed since a lot), how about the approach of checking if the app facade already exists?

Not sure if you meant to tag Will or me.

I'm fine with the change. I'm not using this package currently, so it's not a pressing concern for me.