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

[Bug] usage of incorrect "set_error_handler" interface #198

Closed
wants to merge 2 commits into from

Conversation

YuraLukashik
Copy link
Contributor

It fails with the only error code.
http://php.net/manual/ru/function.set-error-handler.php

@codecov-io
Copy link

codecov-io commented Dec 6, 2016

Current coverage is 96.73% (diff: 100%)

Merging #198 into master will decrease coverage by <.01%

@@             master       #198   diff @@
==========================================
  Files            20         20          
  Lines           645        644     -1   
  Methods         162        162          
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits            624        623     -1   
  Misses           21         21          
  Partials          0          0          

Powered by Codecov. Last update 2f5cdb1...6c1d8fa

@ezzatron
Copy link
Contributor

ezzatron commented Dec 7, 2016

Thanks for the PR! This should be fine to merge, and I'll try to include it in the next release.

So that I can include some extra tests, would you be able to paste the error handler code that you are using when this error occurs?

@YuraLukashik
Copy link
Contributor Author

YuraLukashik commented Dec 7, 2016

@ezzatron that began to happen after upgrade to Symfony 3. The handler is symfony debug error handler https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Debug/ErrorHandler.php#L368 and i think they updated it's interface.

@YuraLukashik
Copy link
Contributor Author

screen shot 2016-12-07 at 15 23 46

@YuraLukashik
Copy link
Contributor Author

@ezzatron it seems that i just had no errors before :)

@YuraLukashik
Copy link
Contributor Author

this bug is reproducible only in php < 7, because php 7.* has Throwable interface which catches such errors.

@YuraLukashik
Copy link
Contributor Author

@ezzatron added test, but it's for specific php versions.

@ezzatron
Copy link
Contributor

ezzatron commented Dec 7, 2016

@YuraLukashik Thanks, that helps a lot!

@ezzatron
Copy link
Contributor

@YuraLukashik After looking through Symfony 3's error handler, it seems like they're using even more arguments than the 5 standard ones. I think, based upon that, it's probably wise just to pass on ALL arguments received by the handler. This way we are as transparent as possible.

I've created a branch with your commits plus some changes and more tests on the error-handler-signature branch. If you'd like to try this branch, you can do so using this version constraint:

{
    "peridot-php/peridot": "dev-error-handler-signature"
}

Is there any chance you could try this branch and let me know if you have any further issues?

Also, just wondering about this statement:

this bug is reproducible only in php < 7, because php 7.* has Throwable interface which catches such errors.

It seems to me like this has nothing to do with PHP 7, but if I'm missing something, perhaps you could explain this a bit more? In any case, I was able to remove the version restrictions from the spec you wrote, and with some minor adjustments, it passes just fine in all versions of PHP and HHVM.

@ezzatron
Copy link
Contributor

ezzatron commented Jan 6, 2017

Manually merged in 88d93d8.

@ezzatron ezzatron closed this Jan 6, 2017
@ezzatron ezzatron added this to the Next release milestone Jan 6, 2017
@ezzatron ezzatron added the bug label Jan 6, 2017
@ezzatron
Copy link
Contributor

@YuraLukashik This PR is scheduled for release at the end of this week, if you have any more comments or questions, it would be good to address them before then. Cheers!

@ezzatron
Copy link
Contributor

Released in 1.19.0 🎉

@YuraLukashik
Copy link
Contributor Author

@ezzatron thank you! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants