Framework: Breaking change introduced in v6.18.12 via #32585: Cache Faker Instances

[shrink]

• Laravel Version: v6.18.12
• PHP Version: 7.3

Description:

As of PR #32585 faker instances are now cached. This is a breaking change as Faker has functionality that maintains state (e.g: unique()) which means state is now persisted between tests. This has broken our tests -- we rely on unique values being reset between tests.

This change should be reverted and go out in future as part of a new major version, or perhaps we can add something as a patch that resets Faker's state between tests.

For anyone who comes across this issue because they're encountering broken tests with faker, you can fix it by reverting the behaviour, register the following:

$this->app->singleton(\Faker\Generator::class, function ($app, $parameters) {
    return \Faker\Factory::create(
        $parameters['locale'] ?? $app['config']->get('app.faker_locale', 'en_US')
    );
});

$this->app->singleton(\Illuminate\Database\Eloquent\Factory::class, function ($app) {
    return \Illuminate\Database\Eloquent\Factory::construct(
        $app->make(\Faker\Generator::class),
        $this->app->databasePath('factories')
    );
});

[deleugpn]

I've been investigating this for the past hour and was about to open an issue for this.

Summary:

Faker has memory leak which the author suggests caching: https://github.com/fzaninotto/Faker/pull/1730
@bytestream recommends Laravel to start caching Faker instance: https://github.com/laravel/framework/issues/32574
Laravel starts caching: https://github.com/laravel/framework/commit/222823377c936ab4cceeb1fa42db84821c04bff6
Change is backported to Laravel 6: https://github.com/laravel/framework/pull/32585

[bytestream]

Surely persisting state is intended if you're using unique() but agree it's an unintended BC break.

Looks like state can be reset using $faker->unique(true) in setUp / tearDown?

https://github.com/fzaninotto/Faker/blob/1d996514052fa7dc75077ce5edba85672eb071be/src/Faker/Provider/Base.php#L576

[bytestream]

Unconfirmed, just reading the code, probably a similar issue with $faker->valid()

https://github.com/fzaninotto/Faker/blob/1d996514052fa7dc75077ce5edba85672eb071be/src/Faker/Provider/Base.php#L608

[deleugpn]

I'm not sure $faker->valid() would classify as the same. It seems something that doesn't hold state across multiple calls within the same test? Each time the valid modifier is used, we're suppose to specify a validator which differs from unique that two different calls have to hold the same state.

[shrink]

Surely persisting state is intended if you're using unique() but agree it's an unintended BC break.

Although I can't speak for all test suites, typically the intention of unique() is to ensure a unique value within the current assertion context but not within the whole test suite's execution.

For example:

$factory->define(Model::class, function (Faker $faker) {
    return [
        'name' => $faker->unique()->randomElement(['a', 'b', 'c']);
    ];
});

The expectation then is that each test could create _up to_ 3 Model::class with a unique name.

public function testExampleA(): void
{
    $models = factory(Model::class, 3)->create();
}

public function testExampleB(): void
{
    $models = factory(Model::class, 3)->create();
}

The suggestion to use unique($reset = true) is a good one -- #32703 seems like the right solution.

[GrahamCampbell]

I don't think this is a breaking change, we just need Laravel to reset uniqueness in between tests. That is, this is a bug, not a breaking change.