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

Silenced E_USER_* are ignored and triggers error/warning/notice/deprecation (display/failing) #5325

Closed
sbuerk opened this issue Apr 13, 2023 · 5 comments
Labels
type/bug Something is broken version/10 Something affects PHPUnit 10

Comments

@sbuerk
Copy link
Contributor

sbuerk commented Apr 13, 2023

Q A
PHPUnit version 10.0, 10.1-dev
PHP version 8.1.x, 8.2.x
Installation Method Composer & PHAR

Summary

Basically, there are two ways to silence error levels:

  • set error_reporting to a value, not containing the error levels which should be silenced/not displayed
  • using the silence operator @ to enforce silencing the error level, unrelated to the set error reporting level

For example, it's possible that E_ALL has been set
as global error_level() and therefore triggering a
user deprecation/warning/error using the trigger_error
method will be displayed. If the @ silence operator
is used before the trigger_error() call, the error
is silenced.

error_reporting(E_ALL);
trigger_error('', E_USER_DEPRECATED);  // will be shown
@trigger_error('', E_USER_DEPRECATED); // will not be shown

However, registered error handler will be called for all errors, despite it has been silenced or not for both possible ways to silence a error levels. ErrorHandler check the
passed $errorNumber against the result of error_reporting() to determine if the errorNumber/errorLevel is supressed or not. That check will also detect a errorNumber as beeing
silenced if the @ silence operator has been used, which could and should be taken as a intentional and enforced silencing of an error (knowing that it would fail).

There are multple use-cases, where for e.g. silenced user deprecation should not be taken as such, displayed and fail.

MockBuilder converts @deprecated method docblock annotations to silenced @trigger_error('', E_USER_DEPRECATED) calls. Having a @deprecated annonation does not mean, that the original method call would trigger an E_USER_DEPRECATED error - and it's arguable that it should be a hard enforcement by PHPUnit to decide this and doing the conversion.

Using the <source> configuration does not help here to let silenced error levels in third party code be ignored but in own code triggerd - or own user errors ignored but 3rd party code ignored. Beside this, it's not possible to silence the silenced E_USER_DEPRECATION triggered by the converted @deprecected annotations of mocked class methods.

Current behavior

Silence state of E_USER_* errors are ignored, regardless if the have been silenced by PHP error level configuration or the @ silence operator before the trigger_error() call.

PHPUnit v9 instead respectes enforced silenced user error levels.

How to reproduce

Using one of these calls in a class method:

<?php

@trigger_error('some message', E_USER_DEPRECATED);
@trigger_error('some message', E_USER_NOTICE);
@trigger_error('some message', E_USER_WARNING);

Having a class:

<?php

class SomeClass {

  public function __construct()
  {
    $this->someMethod();
  }

  /**
   * @deprecated since 4.3, will be removed in 5. Not triggering an error.
   */
  public function someMethod(): void
  {}
}

and creating a mock of it:

$mock = $this->createMock(SomeClass::class);

triggers a suppressed E_USER_DEPRECATED error. The suppressing is ignored,
and the deprecation is collected, displayed if selected and/or failing (exit
code > 0) if selected.

Expected behavior

  • MockBuilder should not decide on its own to trigger a deprecation for a method having only a deprecation annotation

And/or

  • A silenced/enforced silenced error/user error should not be ignored - or it should be configurable (per level/code place/etc).

As a intermediate solution ignoring a suppressed E_USER_* should be removed again, until a workable solution can be found to distingush with all containing parts. This reverts to how it has worked in PHPUnit v9.

@sbuerk sbuerk added type/bug Something is broken version/10 Something affects PHPUnit 10 labels Apr 13, 2023
@sebastianbergmann
Copy link
Owner

MockBuilder converts @deprecated method docblock annotations to silenced @trigger_error('', E_USER_DEPRECATED) calls.

Neither PHPUnit's MockBuilder nor any other part of PHPUnit acts on @deprecated annotations.

@sbuerk
Copy link
Contributor Author

sbuerk commented Apr 13, 2023

Neither PHPUnit's MockBuilder nor any other part of PHPUnit acts on @deprecated annotations.

That's not true. The MockBuilder reads @deprecated docblocks for methods and adds a @trigger_error($deprectedDocBlockMessage, E_USER_DEPRECATED) to the mocked method code.

See:

if (is_string($docComment) &&

@trigger_error({deprecation}, E_USER_DEPRECATED);

'deprecation' => $deprecation,

@sebastianbergmann
Copy link
Owner

Neither PHPUnit's MockBuilder nor any other part of PHPUnit acts on @deprecated annotations.

That's not true. The MockBuilder reads @deprecated docblocks for methods and adds a @trigger_error($deprectedDocBlockMessage, E_USER_DEPRECATED) to the mocked method code.

Thank you so much for your persistence! I had totally forgotten about this arcane bit.

@sbuerk
Copy link
Contributor Author

sbuerk commented Apr 13, 2023

Thank you so much for your persistence! I had totally forgotten about this arcane bit.

You are welcome. We all know these kind of old code not having in mind and breaking someones neck.

I'm not sure what would be really the solution - I'm open to come to a solution everyone is happy.

However, I would suggest to revert the ignoring of the E_USER_* silencing for now - what do you think ? For that I created a change and PR will be incoming.

Sadly, in generall I would help more for another solution .. and we can discuss this. But in the open source project where I'm in the core team, I have quite some tasks to do for the upcoming LTS version beside a lot of stuff on my daytime job. So I have to switch back to the other project.

Can we come somehow come together ? Special for the 10.1 release ?

sebastianbergmann pushed a commit that referenced this issue Apr 14, 2023
Basically, there are two ways to silence error levels:

* set error_reporting to a value, not containing the
  error levels which should be silenced/not displayed
* using the silence operator `@` to enforce silencing
  the error level, unrelated to the set error reporting
  level

For example, it's possible that `E_ALL` has been set
as global error_level() and therefore triggering a
user deprecation/warning/error using the `trigger_error`
method will be displayed. If the `@` silence operator
is used before the `trigger_error()` call, the error
is silenced.

```php
error_reporting(E_ALL);
trigger_error('', E_USER_DEPRECATED);  // will be shown
@trigger_error('', E_USER_DEPRECATED); // will not be shown
```

However, registered error handler will be called for all
errors, despite it has been silenced or not for both possible
ways to silence a error levels. ErrorHandler check the
passed `$errorNumber` against the result of error_reporting()
to determine if the errorNumber/errorLevel is supressed or
not. That check will also detect a errorNumber as beeing
silenced if the `@` silence operator has been used, which
could and should be taken as a intentional and enforced
silencing of an error (knowing that it would fail).

There are multple use-cases, where for e.g. silenced user
deprecation should not be taken as such, displayed and fail.

MockBuilder converts `@deprecated` method docblock annotations
to silenced `@trigger_error('', E_USER_DEPRECATED)` calls.
Having a `@deprecated` annonation does not mean, that the
original method call would trigger an `E_USER_DEPRECATED`
error - and it's arguable that it should be a hard enforcement
by PHPUnit to decide this and doing the conversion.

Therefore, this change removes the possible silenceable error
level list check from the `ErrorHander`. This restores how it
has worked in PHPUnit v9 without opening follow up issues.

This can be reintroduced, either in a configurable way or the
same if related questions has been properly thought about and
solved before:

* MockBuilder @Deprecation -> trigger_error() configurable or
  removed
* proper way to configure between own code / 3rd party code
  if silenced error levels should be stay silenced or not.
  This means not only in one direction.

Related: #5325
@lolli42
Copy link
Contributor

lolli42 commented Apr 15, 2023

Thanks!

With this in place, we finished the migration to phpunit 10.1.

We're happy phpunit can exit non-zero again in case a test calls a method that contains trigger_error('message', E_USER_DEPRECATED):
Our deprecation policy requires we can only deprecate stuff when it's unused. This forces us to get rid of usages first, simplifies removal later, and helps us coming up with good migration documentation for 3rd party consumers. Tests of deprecated stuff is moved to a different test directory with the deprecation patch, that suite is configured to not fail on deprecation, while the main suite does fail when code directly or indirectly calls deprecated things. This strategy works out pretty well for us, and it's good phpunit keeps support for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/bug Something is broken version/10 Something affects PHPUnit 10
Projects
None yet
Development

No branches or pull requests

3 participants