Skip to content

Support faker locales#5456

Closed
gianmaria88 wants to merge 1 commit intocodeigniter4:developfrom
gianmaria88:patch-1
Closed

Support faker locales#5456
gianmaria88 wants to merge 1 commit intocodeigniter4:developfrom
gianmaria88:patch-1

Conversation

@gianmaria88
Copy link
Copy Markdown

static::faker doesn't support selecting locales. Proposed change adds an optional way to specify a locale. Default locale is 'en'.

Each pull request should address a single issue and have a meaningful title.

Description
Explain what you have changed, and why.

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

static::faker doesn't support selecting locales. Proposed change adds an optional way to specify a locale. Default locale is 'en'.
@kenjis
Copy link
Copy Markdown
Member

kenjis commented Dec 12, 2021

@MGatner This is a breaking change. But it is only affect testing code.
So I think a change like this is acceptable in 4.x.
What do you think?

@kenjis
Copy link
Copy Markdown
Member

kenjis commented Dec 12, 2021

@gianmaria88 Thank you for sending the PR.

Fabricator respects $defaultLocale in the Config/App.php.
https://codeigniter4.github.io/CodeIgniter4/testing/fabricator.html#localization

I think it is better this has the same functionality.
What do you think?

@kenjis kenjis added enhancement PRs that improve existing functionalities breaking change Pull requests that may break existing functionalities labels Dec 12, 2021
@digitall-it
Copy link
Copy Markdown

digitall-it commented Dec 12, 2021

@kenjis Actually you can create a Faker in a Seeder class without having to use the Fabricator at all, so the behavior seems inconsistent. Also, as all that Seeder does is to instantiate a Faker singleton, it makes no sense to hide the ability to specify the locale. This would also be helpful in testing apps with multiple locales (i.e. international, internationalized apps), as you may need to seed data in different formats (bank accounts numbers, social security numbers) to check the code against.

At that point, however, one can argue that you can simply instantiate Faker directly in your test class, not use the parent factory, and have access to everything, but nowhere in the documentation is stated that if you use the seeder's faker instance you lose the ability to set the locale.

Also, seems like if you use Faker in a Seeder (not in a Fabricator), it doesn't seem to respect AppLocale (is that by design?), but that's another story.

@MGatner
Copy link
Copy Markdown
Member

MGatner commented Dec 12, 2021

I honestly was not aware this was here. I am not sure the history of this but it makes me uneasy to have a development dependency in a production class. A developer could use self::fake() in a Seeder with everything working fine then deploy to production with composer --no-dev and crash the app.

Given that (as @gianmaria88 noted) all this does is instantiates a Faker instance I would be in favor of deprecating this method and removing it in the future.

I like the idea of offering a Config-aware Faker service but it should be housed out of the CodeIgniter\Test namespace and follow the regular dev/prod division of use.

@MGatner
Copy link
Copy Markdown
Member

MGatner commented Dec 12, 2021

FWIW I usually add this to my own projects. Here's one from a private repo:

<?php

namespace Config;

use CodeIgniter\Config\Services as CoreServices;
use Faker\Factory;
use Faker\Generator;

/**
 * Services Configuration file.
 *
 * Services are simply other classes/libraries that the system uses
 * to do its job. This is used by CodeIgniter to allow the core of the
 * framework to be swapped out easily without affecting the usage within
 * the rest of your application.
 *
 * This file holds any application-specific services, or service overrides
 * that you might need. An example has been included with the general
 * method format you should use for your service methods. For more examples,
 * see the core Services file at system/Config/Services.php.
 */
class Services extends CoreServices
{
    /**
     * Creates a Faker instance for generating random test data.
     */
    public static function faker(?string $locale = null, bool $getShared = true): Generator
    {
        if ($getShared) {
            return static::getSharedInstance('faker', $locale);
        }

        return Factory::create($locale ?? config('App')->defaultLocale);
    }
}

@digitall-it
Copy link
Copy Markdown

I noted that the code actually checked for class availability, however it doesn't seem to use Faker at all, just instance it (with limitations). I second the suggestion to deprecate and then remove it, and update consequently the documentation (and test examples).

@MGatner
Copy link
Copy Markdown
Member

MGatner commented Dec 13, 2021

Checking for the class is "good" but it doesn't do anything with that info, just causes the return to be null. Which means every single developer would have to write their own null checks anytime they use this method. This should have throw an exception if the class didn't exist. Or (as I think we are deciding) this method never should have existed to begin with!

@kenjis
Copy link
Copy Markdown
Member

kenjis commented Dec 14, 2021

I would be in favor of deprecating this method and removing it in the future.

I agree with it.

@MGatner
Copy link
Copy Markdown
Member

MGatner commented Dec 14, 2021

I looked back and it was @paulbalandan that introduced this originally - let's give him a chance a weigh in first.

@paulbalandan
Copy link
Copy Markdown
Member

I looked back and it was @paulbalandan that introduced this originally - let's give him a chance a weigh in first.

I cannot remember the use case why I proposed that method in the first place but if the consensus is to deprecate it then remove eventually then it is fine with me.

kenjis added a commit to kenjis/CodeIgniter4 that referenced this pull request Dec 21, 2021
@kenjis kenjis mentioned this pull request Dec 21, 2021
4 tasks
@kenjis
Copy link
Copy Markdown
Member

kenjis commented Dec 21, 2021

I sent a PR #5490 deprecating Seeder::faker().

@kenjis
Copy link
Copy Markdown
Member

kenjis commented Dec 21, 2021

Although you have sent us the PR, we have decided to be deprecated the method.
Please instantiate Faker by yourself freely.

@kenjis kenjis closed this Dec 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking change Pull requests that may break existing functionalities enhancement PRs that improve existing functionalities

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants