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

E_USER_DEPRECATED errors bypass error_level setting #3424

Closed
jstewmc opened this Issue Aug 9, 2016 · 7 comments

Comments

Projects
None yet
3 participants
@jstewmc
Contributor

jstewmc commented Aug 9, 2016

It appears that Codeception's Subscriber\ErrorHandler doesn't respect the error_level configuration setting for E_USER_DEPRECATION errors.

Background

After a recent composer update, one of our third-party libraries started triggering E_USER_DEPRECATED errors. These errors appeared as notifications after our tests.

I tried setting Codeception's error_level to E_ALL & ~E_STRICT & ~E_DEPRECATED & ~E_USER_DEPRECATED (note the addition of E_USER_DEPRECATED). However, the notifications continued to appear.

config

notifications

I added var_dump(error_reporting()) to my test suite's _beforeSuite() and _afterSuite() methods. Both methods printed 6143, the correct value for E_ALL & ~E_STRICT & ~E_DEPRECATED & ~E_USER_DEPRECATED.

Error reporting appears to be correctly set from start to finish.

Issue

The issue appears to stem from the Subscriber\ErrorHandler.

As I understand it, setting PHP's error level via error_reporting() will set the error levels for PHP's native error handler. However, when you register a custom error handler via the set_error_handler() function, the native error handlers will be bypassed for all the custom handler's error types. If you don't specify error types, a custom error handler defaults to E_ALL | E_STRICT.

On line 36 of the ErrorHandler object, we correctly set the error reporting level to my configuration setting, E_ALL & ~E_STRICT & ~E_DEPRECATED & ~E_USER_DEPRECATED. However, on line 41, we register an error handler without specifying the error types.

handle

Now, in the errorHandler() method, we do check to be sure the error being handled is one we've set with error_reporting(), however, this occurs after the check for E_USER_DEPRECATED errors.

errorhandler

Even though I've excluded E_USER_DEPRECATED errors from Codeception's error_level reporting, they are being reported. That's not great.

Solutions

In my mind, there are two solutions:

Specify the handler's error types

We could specify the custom error handler's error types when we set it. This would remove the need to check the error types in the errorHandler() method.

solution1

Move the E_USER_DEPRECATED check

We could move the E_USER_DEPRECATED check in the errorHandler method below the error types check.

solution2

Either solution fixes the issue.

success

Pull request

The first solution seems more correct. We shouldn't handle events only to not handle them, unless we want to suppress PHP's native error handling. The second solution is definitely a much smaller change.

I'm happy to make a pull request for either one. I wanted to check with the authors/contributors first, because there is plenty going on in here that I don't understand haha.

Which solution do you recommend?

@Naktibalda

This comment has been minimized.

Show comment
Hide comment
@Naktibalda

Naktibalda Aug 9, 2016

Member

I think that this is by design to support Symfony Depreciations Helper.

To make your tests pass you have to set environment variable SYMFONY_DEPRECATIONS_HELPER to weak, like here.

Member

Naktibalda commented Aug 9, 2016

I think that this is by design to support Symfony Depreciations Helper.

To make your tests pass you have to set environment variable SYMFONY_DEPRECATIONS_HELPER to weak, like here.

@jstewmc

This comment has been minimized.

Show comment
Hide comment
@jstewmc

jstewmc Aug 10, 2016

Contributor

Oh, cool. Thank you for the suggestion.

I don't use Symfony Deprecations Helper in my project, but I tried setting the SYMFONY_DEPRECATIONS_HELPER=weak environment variable in my Codeception bootstrap anyway.

bootstrap

Unfortunately, it did not fix the issue.

You're right about supporting Symfony Deprecations Helper in the ErrorHandler object. Deprecation errors appear to be special. Their error handler is registered separately than the other error handlers.

handle

It looks like the environment variable only matters if the Symfony PHPUnit Bridge component is installed, though (which makes sense).

registerdeprecationerrorhandler

I'm not dying to install a library just to turn it off (haha). I think Solution 2 from above (moving the E_USER_DEPRECATED check in errorHandler() below the $errno check) would fix the issue.

What do you think?

Oh, and thank you for always being so helpful and responsive with issues. Thank you and the Codeception team for a great testing framework!

Contributor

jstewmc commented Aug 10, 2016

Oh, cool. Thank you for the suggestion.

I don't use Symfony Deprecations Helper in my project, but I tried setting the SYMFONY_DEPRECATIONS_HELPER=weak environment variable in my Codeception bootstrap anyway.

bootstrap

Unfortunately, it did not fix the issue.

You're right about supporting Symfony Deprecations Helper in the ErrorHandler object. Deprecation errors appear to be special. Their error handler is registered separately than the other error handlers.

handle

It looks like the environment variable only matters if the Symfony PHPUnit Bridge component is installed, though (which makes sense).

registerdeprecationerrorhandler

I'm not dying to install a library just to turn it off (haha). I think Solution 2 from above (moving the E_USER_DEPRECATED check in errorHandler() below the $errno check) would fix the issue.

What do you think?

Oh, and thank you for always being so helpful and responsive with issues. Thank you and the Codeception team for a great testing framework!

@Naktibalda

This comment has been minimized.

Show comment
Hide comment
@Naktibalda

Naktibalda Aug 10, 2016

Member

@raistlin any comments?

Member

Naktibalda commented Aug 10, 2016

@raistlin any comments?

@Naktibalda

This comment has been minimized.

Show comment
Hide comment
@Naktibalda

Naktibalda Aug 19, 2016

Member

In my opinion, deprecation messages should always be visible to encourage users to fix them.
However your situation could be improved by removing duplicate messages.

Member

Naktibalda commented Aug 19, 2016

In my opinion, deprecation messages should always be visible to encourage users to fix them.
However your situation could be improved by removing duplicate messages.

@jstewmc

This comment has been minimized.

Show comment
Hide comment
@jstewmc

jstewmc Aug 19, 2016

Contributor

I'm right there with you. Normally, I want to see deprecation messages.

Unfortunately, these notices are triggered and caused by libraries outside of my control. In this case, I'd just like to turn the notices off, because I can't fix them.

In my opinion, the Codeception error_level configuration setting should be respected for all errors, even E_DEPRECATION and E_USER_DEPRECATION notices. That way, the user can decide what errors should be visible. That's why I opened the issue and submitted PR #3445.

If you guys don't agree, it's no big deal. I just saw something I thought I could improve at a difficulty level I figured I could handle haha.

Contributor

jstewmc commented Aug 19, 2016

I'm right there with you. Normally, I want to see deprecation messages.

Unfortunately, these notices are triggered and caused by libraries outside of my control. In this case, I'd just like to turn the notices off, because I can't fix them.

In my opinion, the Codeception error_level configuration setting should be respected for all errors, even E_DEPRECATION and E_USER_DEPRECATION notices. That way, the user can decide what errors should be visible. That's why I opened the issue and submitted PR #3445.

If you guys don't agree, it's no big deal. I just saw something I thought I could improve at a difficulty level I figured I could handle haha.

@wojtas

This comment has been minimized.

Show comment
Hide comment
@wojtas

wojtas Aug 22, 2016

I definitely agree with @jstewmc on this matter. In my case I get a lot of deprecation warnings which are triggered by code I have totally no control over; I think it just pollutes the test report. I think that the user should be able to decide whether he wants to be notified of deprecations while testing or not. In my opinion, moving the aforementioned part of the code in the ErrorHandler::errorHandler method would be the best solution. In such cases, configurability is strongly desirable.

wojtas commented Aug 22, 2016

I definitely agree with @jstewmc on this matter. In my case I get a lot of deprecation warnings which are triggered by code I have totally no control over; I think it just pollutes the test report. I think that the user should be able to decide whether he wants to be notified of deprecations while testing or not. In my opinion, moving the aforementioned part of the code in the ErrorHandler::errorHandler method would be the best solution. In such cases, configurability is strongly desirable.

@Naktibalda

This comment has been minimized.

Show comment
Hide comment
@Naktibalda

Naktibalda Aug 22, 2016

Member

I agree that error_level setting should be respected.

@jstewmc I will accept a patch which assigns the result of eval("return {$this->errorLevel};") to private property and uses it instead of error_reporting() in errorHandler.

Member

Naktibalda commented Aug 22, 2016

I agree that error_level setting should be respected.

@jstewmc I will accept a patch which assigns the result of eval("return {$this->errorLevel};") to private property and uses it instead of error_reporting() in errorHandler.

Naktibalda added a commit to Naktibalda/Codeception that referenced this issue Aug 22, 2016

Naktibalda added a commit to Naktibalda/Codeception that referenced this issue Aug 23, 2016

@DavertMik DavertMik closed this in #3460 Aug 26, 2016

DavertMik added a commit that referenced this issue Aug 26, 2016

Handle deprecation messages according to error_level setting (#3460)
* All error messages are handled according to error_level setting

Fixes #3424

* Only E_USER_DEPRECATED messages will respect error_level setting

Fixes #3424

* Changed assertion to make test easier to debug

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