Skip to content

Conversation

nicolas-grekas
Copy link
Member

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? yes
Tests pass? yes
Fixed tickets none
License MIT
Doc PR none

Enhance error handling and thus displaying for catchable PHP errors.
Code is tricky thanks to https://bugs.php.net/54275

Before:

capture du 2014-04-17 10 03 40

After:

capture du 2014-04-17 10 03 14

@nicolas-grekas nicolas-grekas changed the title Handled errors [Debug] Handled errors Apr 17, 2014
@Koc
Copy link
Contributor

Koc commented Apr 17, 2014

It looks rather than bugfix than new feature). Any chance merge this ti 2.3/2.5?

@nicolas-grekas
Copy link
Member Author

Did it work before: not ideally from a user land pov, but yes => not a bug
Does it work better now: yes => a new feature

@stof
Copy link
Member

stof commented Apr 17, 2014

I think this could still go in 2.5 as part of the stabilization phase

@nicolas-grekas
Copy link
Member Author

Just retagged as bug fix, hopping like you that it can go through 2.5 ;-)

I also pushed an important code update: fatal errors also are now re-injected in the HttpKernel->handleException + HttpKernel::terminate code path. That means that KernelEvents::EXCEPTION + KernelEvents::RESPONSE + KernelEvents::TERMINATE are triggered even for fatal errors.

As a consequence, the web toolbar and all the other usual sweetness will work again.

The code is resistant to any second fatal error that might happen while doing this re-injection thanks to a pre-buffering technique implemented by HandledErrorException: in the worst case, the response will be the same degraded one as before.

As a corollary, the stack trace is now fixed for fatal errors when xdebug is enabled.

Before:

capture du 2014-04-18 15 21 01

After:

capture du 2014-04-18 15 21 27

* @param object $object Any object
* @param string $method A method name that will be called on $object with a FatalErrorException as argument
*/
public static function setFatalErrorExceptionHandler($object, $method)
Copy link
Member

Choose a reason for hiding this comment

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

Please use a single argument accepting a callable. This way, any callable will be supported (the code already supports all types of callables internally)

Copy link
Member Author

Choose a reason for hiding this comment

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

But I need dependency injection from the container builder...

Copy link
Member

Choose a reason for hiding this comment

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

<service>
    <argument type="collection">
        <argument type="service" id="http_kernel" />
        <argument>handleFatalErrorException</argument>
    </argument>
</service>

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree with stof

@romainneutron
Copy link
Contributor

You should probably update changelogs to mention deprecations

$trace = array_slice(array_reverse($trace), 1);
}
}
$trace = array_slice(array_reverse($trace), 1);
Copy link
Member

Choose a reason for hiding this comment

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

this is now sliced for all exceptions, not only fatal ones. Is it intended ?

@fabpot
Copy link
Member

fabpot commented Apr 18, 2014

... and tests do not pass.

@nicolas-grekas
Copy link
Member Author

Hum... fixed!

@fabpot
Copy link
Member

fabpot commented Apr 19, 2014

Great job @nicolas-grekas

@fabpot fabpot closed this in 27e1a89 Apr 19, 2014
@Tobion
Copy link
Contributor

Tobion commented Apr 19, 2014

Since several phpdoc were missing, I wouldn't have merged it yet.

@nicolas-grekas nicolas-grekas deleted the handled-errors branch April 22, 2014 16:26
fabpot added a commit that referenced this pull request Apr 28, 2014
…t/54275 (nicolas-grekas)

This PR was merged into the 2.5-dev branch.

Discussion
----------

[Debug] less intrusive work around for https://bugs.php.net/54275

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #10787, #10292
| License       | MIT
| Doc PR        | none

- This PR supersedes the behavior introduced in #10725 :
  Instead of building some complicated code to work around https://bugs.php.net/54275, the code is now as
  straightforward as possible, with a conditional fallback work around.
- The handling of fatal errors is also made more robust/fail-safe.
- Last but not least, ErrorsLoggerListener and FatalErrorExceptionsListener are now registered earlier and
  are now cleaning up their handler/logger once set to prevent setting again and again for sub-requests
  (+remove one refcount for these handler and logger).

Commits
-------

d7a186f [Debug] less intrusive work around for https://bugs.php.net/54275
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.

6 participants