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

Can't override default formatters #435

Closed
ZhukV opened this issue Jan 25, 2022 · 9 comments · Fixed by #441
Closed

Can't override default formatters #435

ZhukV opened this issue Jan 25, 2022 · 9 comments · Fixed by #441
Labels
bug Something isn't working help wanted Extra attention is needed

Comments

@ZhukV
Copy link

ZhukV commented Jan 25, 2022

Previously (to #267) we can change the default logic and formatters in our application by providers. As an example, we override uuid function for correct return UuidInterface (object instead of string). All correct work previously, because our provider have large priority than default provider for uuid. After merge #267, previously you check the method existence in generator, and only if method not exist, check in providers. As result, if method uuid exist in generator class, our provider don't called never.

Any solution for override default generator logic?

Thank, have a nice day!

@pimjansen
Copy link

Do you have some code on how you did this in the past?

Please also provide Faker and PHP version as the templates suggest

@localheinz
Copy link
Member

@pimjansen

Probably something similar to how it is done in #386.

@ZhukV
Copy link
Author

ZhukV commented Jan 25, 2022

@localheinz , no, #386 is a different problem. I will provide the code in a while.

@ZhukV
Copy link
Author

ZhukV commented Jan 25, 2022

<?php

namespace Acme;

use Faker\Generator;
use Faker\Provider\Base;
use Ramsey\Uuid\Uuid;
use Ramsey\Uuid\UuidInterface;

include_once __DIR__.'/vendor/autoload.php';

class CommonProvider extends Base
{
    public function uuid(): UuidInterface
    {
        return Uuid::uuid4();
    }
}

$faker = new Generator();
$faker->addProvider(new CommonProvider($faker));

$uuid = $faker->uuid();

var_dump($uuid);

Please try run on faker ~1.17.0 and ~1.18.0.

Output after run in 1.17:

root@79cd49821e12:/code# php test.php 
object(Ramsey\Uuid\Lazy\LazyUuidFromString)#6 (2) {
  ["uuid":"Ramsey\Uuid\Lazy\LazyUuidFromString":private]=>
  string(36) "cc31b52d-4970-4bd3-abc4-b4045677aa3e"
  ["unwrapped":"Ramsey\Uuid\Lazy\LazyUuidFromString":private]=>
  NULL
}

Output after run in 1.18:

root@79cd49821e12:/code# php test.php 
string(36) "d460b553-32ea-34cb-95f2-f84567a68eab"

Problem: to version 1.8 we can manipulate with providers, add custom provider and our added providers added to first, as result, if core provider support uuid, we can replace it with add customer provider with uuid method.
On #267 (here https://github.com/FakerPHP/Faker/pull/267/files#diff-31471b0b25bb74ef183af00be15790b9e3f2c1488a29d7c1f113ef794281c6a3R636) if the method exist in Generator class, the provider not called. It's very difficult to understanding because previously all developer knows - all generated data provided via any providers, and if we want to change logic, or replace or add new, we only add new provider. Now, we must check, exist method or else and if method existence, we can change logic only with extends from base generator.

I think, better solution for this problem - use only providers for generate data, not check by method existence in Generator.

Thank.

@bram-pkg
Copy link
Member

Hello, in an ideal world I agree. However we need to maintain backwards compatibility.

@pimjansen do you think changing the order that it checks for method existance is a breaking change?

For now, you could try our new extension mechanism. For an example, check https://github.com/fakerphp/dutch

It is still experimental, but it is functional.

@pimjansen
Copy link

Hello, in an ideal world I agree. However we need to maintain backwards compatibility.

@pimjansen do you think changing the order that it checks for method existance is a breaking change?

For now, you could try our new extension mechanism. For an example, check https://github.com/fakerphp/dutch

It is still experimental, but it is functional.

Well wel can assume that the latest is always valid right? This is probably happening because the new uuid method is added to the Generator itself. I think the only reason is to overwrite it with the extensions. But you are right they are experimental i would not suggest that.

I need to dive in the code tomorrow to see what the best approach is. @bram-pkg could also mean we revoke the methods on the generator for v1 and use the magic instead. For v2 the core extension interface can be used with psr11

@pimjansen pimjansen added bug Something isn't working help wanted Extra attention is needed labels Jan 25, 2022
@pimjansen
Copy link

Indeed this is what happening, it will access the method directly under the Generator and not invoke the providers due to this. @Nyholm @bram-pkg you got any other idea except revert the add to the generator for V1 and wait for V2 to start implementing those on the Generator itself?

@bram-pkg
Copy link
Member

I think changing the order in which it detects the "Providers" is the best. Putting the provider check above the Generator check.

@pimjansen
Copy link

I think changing the order in which it detects the "Providers" is the best. Putting the provider check above the Generator check.

If ik correct this is not the problem. The providers and all the legacy stuff is access via a __call method which is always last.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants