Bug: errorHandler removes Faker's deprecated errors

[kenjis]

From #6758 (comment)

What happened?

--- 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.

Anything else?

The value of error_reporting() will change in error handler when using @.
See

• https://3v4l.org/0DbSF
• https://3v4l.org/aftRX

If a custom error handler function is set with set_error_handler(), it will still be called even though the diagnostic has been suppressed.

Warning
Prior to PHP 8.0.0, the error_reporting() called inside the custom error handler always returned 0 if the error was suppressed by the @ operator. As of PHP 8.0.0, it returns the value E_ERROR | E_CORE_ERROR | E_COMPILE_ERROR | E_USER_ERROR | E_RECOVERABLE_ERROR | E_PARSE.
https://www.php.net/manual/en/language.operators.errorcontrol.php

[paulbalandan]

Hmm. How do you want to approach this? Always throw even if the error is silenced? Or instead log if silenced? Or else?

[paulbalandan]

To note, in 4.3, these deprecations are loggable now.

> git diff
diff --git a/app/Config/Exceptions.php b/app/Config/Exceptions.php
index ca0713a33..3e5beffc2 100644
--- a/app/Config/Exceptions.php
+++ b/app/Config/Exceptions.php
@@ -60,7 +60,7 @@ class Exceptions extends BaseConfig
      * Use this option to temporarily cease the warnings and instead log those.
      * This option also works for user deprecations.
      */
-    public bool $logDeprecationsOnly = false;
+    public bool $logDeprecationsOnly = true;

     /**
      * --------------------------------------------------------------------------
@@ -73,5 +73,5 @@ class Exceptions extends BaseConfig
      * The related `Config\Logger::$threshold` should be adjusted, if needed,
      * to capture logging the deprecations.
      */
-    public string $deprecationLogLevel = LogLevel::WARNING;
+    public string $deprecationLogLevel = LogLevel::ERROR;
 }

In the logs:

ERROR - 2022-10-27 00:44:15 --> [DEPRECATED] Since fakerphp/faker 1.14: Accessing property "randomDigit" is deprecated, use "randomDigit()" instead. in VENDORPATH\symfony\deprecation-contracts\function.php on line 25.
 1 VENDORPATH\symfony\deprecation-contracts\function.php(25): trigger_error('Since fakerphp/faker 1.14: Accessing property "randomDigit" is deprecated, use "randomDigit()" instead.', 16384)
 2 VENDORPATH\fakerphp\faker\src\Faker\Generator.php(950): trigger_deprecation('fakerphp/faker', '1.14', 'Accessing property "%s" is deprecated, use "%s()" instead.', 'randomDigit', 'randomDigit')
 3 ROOTPATH\tests\system\Test\FabricatorTest.php(123): Faker\Generator->__get('randomDigit')
 4 VENDORPATH\phpunit\phpunit\src\Framework\TestCase.php(1548): CodeIgniter\Test\FabricatorTest->testGetFakerReturnsUsableGenerator()
 5 VENDORPATH\phpunit\phpunit\src\Framework\TestCase.php(1154): PHPUnit\Framework\TestCase->runTest()
 6 VENDORPATH\phpunit\phpunit\src\Framework\TestResult.php(728): PHPUnit\Framework\TestCase->runBare()
 7 VENDORPATH\phpunit\phpunit\src\Framework\TestCase.php(904): PHPUnit\Framework\TestResult->run(Object(CodeIgniter\Test\FabricatorTest))
 8 VENDORPATH\phpunit\phpunit\src\Framework\TestSuite.php(673): PHPUnit\Framework\TestCase->run(Object(PHPUnit\Framework\TestResult))
 9 VENDORPATH\phpunit\phpunit\src\Framework\TestSuite.php(673): PHPUnit\Framework\TestSuite->run(Object(PHPUnit\Framework\TestResult))
10 VENDORPATH\phpunit\phpunit\src\Framework\TestSuite.php(673): PHPUnit\Framework\TestSuite->run(Object(PHPUnit\Framework\TestResult))
11 VENDORPATH\phpunit\phpunit\src\TextUI\TestRunner.php(673): PHPUnit\Framework\TestSuite->run(Object(PHPUnit\Framework\TestResult))
12 VENDORPATH\phpunit\phpunit\src\TextUI\Command.php(144): PHPUnit\TextUI\TestRunner->run(Object(PHPUnit\Framework\TestSuite), [...], [], true)
13 VENDORPATH\phpunit\phpunit\src\TextUI\Command.php(97): PHPUnit\TextUI\Command->run([...], true)
14 VENDORPATH\phpunit\phpunit\phpunit(98): PHPUnit\TextUI\Command::main()
15 VENDORPATH\bin\phpunit(123): include('VENDORPATH\\phpunit\\phpunit\\phpunit')

[kenjis]

Since error_reporting(-1) in the test environment, I assumed that all errors would be displayed during the test execution.
However, this error was erased and I did not notice it.

Other deprecated errors will cause the test to fail, and shouldn't the test fail by default?

[MGatner]

See also: https://github.com/symfony/deprecation-contracts

We can declare our own trigger_deprecation() method - seems to be the approved workaround.

[paulbalandan]

See also: https://github.com/symfony/deprecation-contracts

We can declare our own trigger_deprecation() method - seems to be the approved workaround.

I have considered this after the merge of the deprecation logging feature in 4.3. But decided against to because that would entail declaring the framework to "conflicts": { "symfony/deprecation-contracts": "*" } . But we have deps depending on this:

$ composer why symfony/deprecation-contracts
fakerphp/faker           v1.20.0 requires symfony/deprecation-contracts (^2.2 || ^3.0) 
guzzlehttp/guzzle        7.5.0   requires symfony/deprecation-contracts (^2.2 || ^3.0)
symfony/config           v6.1.3  requires symfony/deprecation-contracts (^2.1|^3)
symfony/console          v6.1.6  requires symfony/deprecation-contracts (^2.1|^3)
symfony/options-resolver v6.1.0  requires symfony/deprecation-contracts (^2.1|^3)

[MGatner]

By "approved workaround" I mean it is what Symfony recommends on their own package. I wouldn't consider that a conflict so much as CodeIgniter replacing a library deprecation handler with its own.