Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[6.x] Resolve Faker\Generator out of the container if it is bound #30992

Merged
merged 1 commit into from
Jan 1, 2020
Merged

[6.x] Resolve Faker\Generator out of the container if it is bound #30992

merged 1 commit into from
Jan 1, 2020

Conversation

ejunker
Copy link
Contributor

@ejunker ejunker commented Jan 1, 2020

If Faker\Generator is bound in the container then use that instance instead of creating a default version. This allows you to bind your own custom Faker\Generator and add custom faker providers which will be available when using WithFaker

For example, you can create a FakerServiceProvider that can bind it like this:

        $this->app->singleton(Generator::class, function () {
            $faker = Factory::create();
            $faker->addProvider(new CompanyNameGenerator\FakerProvider($faker));

            return $faker;
        });

The code is adding the custom CompanyNameGenerator faker provider. Since it is bound in the container then it will be used and the custom company name provider will be available to be used. Without this change then the CompanyNameGenerator provider would not be available and would have to be manually added everytime you wanted to use it.

If Faker\Generator is bound in the container then use that instance instead of creating a default version. This allows you to bind your own custom Faker\Generator and add custom faker providers which will be available when using WithFaker
@GrahamCampbell GrahamCampbell changed the title Resolve Faker\Generator out of the container if it is bound [6.x] Resolve Faker\Generator out of the container if it is bound Jan 1, 2020
@GrahamCampbell
Copy link
Member

I don't quite follow why you'd need this. Why not just make your own trait?

@taylorotwell taylorotwell merged commit 50e43a5 into laravel:6.x Jan 1, 2020
@ejunker ejunker deleted the patch-1 branch January 6, 2020 14:29
@telkins
Copy link
Contributor

telkins commented Jan 10, 2020

Not sure what's going on here, but this seems to have broken several dozen unit tests in one of my projects.

Some of my tests use data providers and some of those use faker to generate some test data. $this->faker is not available, so I use $this->makeFaker() to get one to use to generate the test data.

This now fails. I've just started to look at it, but I wanted to raise the flag in case someone can save me time or is also experiencing the same problem. It may be that I need to make changes in my code...or it may be that this PR has caused an issue.

@driesvints
Copy link
Member

@telkins it's already been reported and fixed. Will be in the next release on Tuesday: #31083

@telkins
Copy link
Contributor

telkins commented Jan 10, 2020

Thx @driesvints . I just came back here to link the fix...looks like you've already done so...and I can now see that it was already linked. Could have saved myself a bit of time...! ;-)

I think I'll take a break now.... :-)

@jamesfairhurst
Copy link
Contributor

jamesfairhurst commented Feb 26, 2020

I think this prevents locale changes from working too as it always resolves the en_US providers.

Overriding the trait in a test and attempting to create a specific locale instance doesn't seem to be working for me e.g.

protected function setUpFaker()
{
    $this->faker = $this->makeFaker('en_GB');
}

This is in a package and has been working for me in the past so I think a orchestra/testbench update with a newer version of the framework surfaced the issue. Still looking into it so could be a local issue but just wanted to drop a comment to sense check it.

Edit:
So looks like it's being initially bound to the container in the singleton in DatabaseServiceProvider@registerEloquentFactory which uses the faker factory to bind a generator with the default locale.

$this->app->singleton(FakerGenerator::class, function ($app) {
    return FakerFactory::create($app['config']->get('app.faker_locale', 'en_US'));
});

As the generater doesn't have a constructor attempting to $this->app->make(Generator::class, [$locale]) with locale arguments doesn't have any affect. I did look into using $this->app->extend but as the locale is used heavily in the Faker Factory it didn't make sense.

I would personally revert this commit, although it solves a specific case it's probably best to keep this as generic as possible and if a custom provider is required (like the example driving the change) simply override the setUpFaker method in the TestCase.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants