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

fix: workaround for Faker deprecation errors in PHP 8.2 #6758

Merged
merged 3 commits into from
Oct 27, 2022

Conversation

kenjis
Copy link
Member

@kenjis kenjis commented Oct 26, 2022

Description
See #6170

Suppress the following error in PHP 8.2.
This error prevents our app and library developments to support 8.2. See #6172 (comment)

1) CodeIgniter\Test\FabricatorTest::testMakeArrayReturnsArray
ErrorException: Use of "static" in callables is deprecated

/Users/kenji/work/codeigniter/official/CodeIgniter4/vendor/fakerphp/faker/src/Faker/Provider/Base.php:374
/Users/kenji/work/codeigniter/official/CodeIgniter4/vendor/fakerphp/faker/src/Faker/Provider/Base.php:432
/Users/kenji/work/codeigniter/official/CodeIgniter4/vendor/fakerphp/faker/src/Faker/Provider/Base.php:449
/Users/kenji/work/codeigniter/official/CodeIgniter4/vendor/fakerphp/faker/src/Faker/Provider/Internet.php:118
/Users/kenji/work/codeigniter/official/CodeIgniter4/vendor/fakerphp/faker/src/Faker/Generator.php:696
/Users/kenji/work/codeigniter/official/CodeIgniter4/vendor/fakerphp/faker/src/Faker/Generator.php:744
/Users/kenji/work/codeigniter/official/CodeIgniter4/vendor/fakerphp/faker/src/Faker/Generator.php:747
/Users/kenji/work/codeigniter/official/CodeIgniter4/vendor/fakerphp/faker/src/Faker/Provider/Internet.php:51
/Users/kenji/work/codeigniter/official/CodeIgniter4/vendor/fakerphp/faker/src/Faker/Generator.php:696
/Users/kenji/work/codeigniter/official/CodeIgniter4/vendor/fakerphp/faker/src/Faker/Generator.php:952
/Users/kenji/work/codeigniter/official/CodeIgniter4/system/Test/Fabricator.php:377
/Users/kenji/work/codeigniter/official/CodeIgniter4/tests/system/Test/FabricatorTest.php:254

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

@kenjis kenjis added the PHP 8.2 label Oct 26, 2022
@kenjis kenjis changed the title fix: workaround for Faker deprecation errors in PHP 8.2. fix: workaround for Faker deprecation errors in PHP 8.2 Oct 26, 2022
@kenjis kenjis mentioned this pull request Oct 26, 2022
7 tasks
Copy link
Member

@MGatner MGatner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little uneasy about the vendor-specific exception workaround. That said, we have so few dependencies, and this one is dev only. Let's see what other opinions surface.

@MGatner
Copy link
Member

MGatner commented Oct 26, 2022

Another case against this: it might hide errors that people would actually like to see. This is, after all, what deprecations are for.

For example, did you know Faker has deprecated magic property access? I didn't until something went wrong in some code last week and I accidentally found the deprecations due to a dependency mishap, because of the way they handle them.

// deprecated:
$faker->firstName;
// use:
$faker->firstName()

Ref:

@kenjis
Copy link
Member Author

kenjis commented Oct 26, 2022

The Faker deprecated magic property access error never shows in CI4 testing.
Because error_reporting() value will change at errorHandler().

The trigger_error() is using @:

    function trigger_deprecation(string $package, string $version, string $message, ...$args): void
    {
        @trigger_error(($package || $version ? "Since $package $version: " : '').($args ? vsprintf($message, $args) : $message), \E_USER_DEPRECATED);
    }

In PHP 7.4, @ makes the error_reporting() value 0:

public function errorHandler(int $severity, string $message, ?string $file = null, ?int $line = null)
{
if (error_reporting() & $severity) {
throw new ErrorException($message, 0, $severity, $file, $line);
}
return false; // return false to propagate the error to PHP standard error handler
}

In PHP 8, the error_reporting() value will be 4437. See https://bugs.php.net/bug.php?id=80548

In both cases, E_USER_DEPRECATED error is not shown.

@kenjis
Copy link
Member Author

kenjis commented Oct 26, 2022

--- a/system/Debug/Exceptions.php
+++ b/system/Debug/Exceptions.php
@@ -151,7 +151,7 @@ class Exceptions
      */
     public function errorHandler(int $severity, string $message, ?string $file = null, ?int $line = null)
     {
-        if (error_reporting() & $severity) {
+        if (E_ALL & $severity) {
             throw new ErrorException($message, 0, $severity, $file, $line);
         }
 
$ ./phpunit tests/system/Test/FabricatorTest.php
PHPUnit 9.5.25 #StandWithUkraine

Runtime:       PHP 8.1.11
Configuration: /Users/kenji/work/codeigniter/official/CodeIgniter4/phpunit.xml

.........E...........EEEEEEEEEEEEEEEE..........                   47 / 47 (100%)

Time: 00:00.625, Memory: 18.00 MB

There were 17 errors:

1) CodeIgniter\Test\FabricatorTest::testGetFakerReturnsUsableGenerator
ErrorException: Since fakerphp/faker 1.14: Accessing property "randomDigit" is deprecated, use "randomDigit()" instead.

/Users/kenji/work/codeigniter/official/CodeIgniter4/vendor/symfony/deprecation-contracts/function.php:25
/Users/kenji/work/codeigniter/official/CodeIgniter4/vendor/fakerphp/faker/src/Faker/Generator.php:950
/Users/kenji/work/codeigniter/official/CodeIgniter4/tests/system/Test/FabricatorTest.php:123

...

ERRORS!
Tests: 47, Assertions: 32, Errors: 17.

@kenjis kenjis force-pushed the fix-Faker-php82-deprecation branch from e3d4fa1 to 7bd1ed5 Compare October 27, 2022 00:14
@kenjis
Copy link
Member Author

kenjis commented Oct 27, 2022

Another case against this: it might hide errors that people would actually like to see. This is, after all, what deprecations are for.

I changed to limit only the error Use of "static" in callables is deprecated.
Also, the error is now output to the log.

Copy link
Member

@MGatner MGatner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making it specific. I'm okay with this under the banner of "workaround to support 8.2", since it is actually just a deprecation upstream. Let's be sure to pull this whenever Faker updates.

@kenjis kenjis merged commit 89c8a6c into codeigniter4:develop Oct 27, 2022
@kenjis kenjis deleted the fix-Faker-php82-deprecation branch October 27, 2022 11:43
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.

3 participants