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

Make sure provider methods are callable in Generator::getFormatter() #952

Open
wants to merge 2 commits into
base: 2.0
Choose a base branch
from

Conversation

max-m
Copy link

@max-m max-m commented Feb 27, 2025

What is the reason for this PR?

Currently non-public methods of providers can override public methods of providers below them in the chain.

If accepted, this change can also be backported to 1.x.

  • A new feature
  • Fixed an issue

Author's checklist

Note: Before and after the change, the same 7 test cases failed.

Summary of changes

class ProviderA extends \Faker\Provider\Base {
    public function testFN() {
        echo 'Hello, world!';
    }
}

class ProviderB extends \Faker\Provider\Base {
    private function testFN() {
        echo 'Get off my lawn!';
    }
}

$generator = \Faker\Factory::create();
$generator->addProvider(new ProviderA($generator));
$generator->addProvider(new ProviderB($generator));
$generator->testFN();

This example will throw a fatal error:
call_user_func_array(): Argument #1 ($callback) must be a valid callback, cannot access private method ProviderB::testFN()

If we take a look at the Generator class, we find

Faker/src/Generator.php

Lines 723 to 729 in 36ab451

foreach ($this->providers as $provider) {
if (method_exists($provider, $format)) {
$this->formatters[$format] = [$provider, $format];
return $this->formatters[$format];
}
}

method_exists() also returns true for protected and private methods.
See https://3v4l.org/eXkna

Instead we can use is_callable(), which respects the visibility, see https://3v4l.org/rHeUg

With this change we get Hello, world! as a result. :)

Review checklist

  • All checks have passed
  • Changes are added to the CHANGELOG.md
  • Changes are approved by maintainer

Currently non-public methods of providers can override public methods of providers below them in the chain.

This happens because `method_exists()` also returns `true` for protected and private methods.

Instead we can use `is_callable()`, which respects the visibility.
@max-m
Copy link
Author

max-m commented Feb 28, 2025

I have added a couple of simple tests, but had to comment two of them out because of the protected_to_private CS fixer rule.

vendor/bin/phpunit --filter "Generator"
PHPUnit 9.6.22 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.2.7
Configuration: Faker/phpunit.xml.dist
Random Seed:   1740756979

Testing
......................................38 / 38 (100%)

Time: 00:00.040, Memory: 8.00 MB

OK (38 tests, 49 assertions)

Legacy deprecation notices (7)

This run included the testProtectedProviderMethodsAreNotCalled() and testProtectedProviderMethodsDoNotShadowPublicMethods() tests to make sure they work as expected.

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.

1 participant