Skip to content

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.

@stale
Copy link

stale bot commented Apr 25, 2025

This issue has been automatically marked as stale because it has not had recent activity. It will be closed after 1 week if no further activity occurs. Thank you for your contributions.

@stale stale bot closed this May 6, 2025
@max-m
Copy link
Author

max-m commented May 6, 2025

:(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant