Skip to content

[HttpKernel] Fix php finally executing before handleThrowable() #47848

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

Closed
wants to merge 1 commit into from

Conversation

gnat42
Copy link
Contributor

@gnat42 gnat42 commented Oct 12, 2022

Q A
Branch? 6.1
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix #47405 Fix #47358
License MIT
Doc PR

Commit e35d6ba causes some odd behaviour. What seems to be happening is a that the requestStack->pop() in the original commit in the finally{} catch is being executed before the handleThrowable() is called. This is most evident in #47358 where the LogoutUrlGenerator attempts to get a request from the requestStack but it is gone.

I verified using xdebug and breakpoints that the code in finally was being called first. With this patch, the original tests in e35d6ba continue to pass and my project where I reported the issue #47358 is not longer occurring.

@wouterj
Copy link
Member

wouterj commented Oct 12, 2022

Thanks for investigating this and creating a patch!

Commit e35d6ba causes some odd behaviour. What seems to be happening is a that the requestStack->pop() in the original commit in the finally{} catch is being executed before the handleThrowable() is called.

I digged into this a bit more, as I don't like odd behavior 😃

What is important here is that the catch was only catching Exception, not Error. The one triggering handleThrowable() is a custom error handler. Apparently, if the try { ... } block contains a PHP error, the finally block is run before the error handler. I can reproduce this in a simple example: https://3v4l.org/mfgII

Unfortunately, I don't think we want to change the catch to \Exception|\Error like you did in this patch. This means the whole logic suddenly gets errors as well, instead of exceptions. That seems like a big BC break to me.

However, I'm unsure what to do here. To me, it seems like we should revert e35d6ba and find a way to somehow pop the request stack for errors as well, without finally block. Let's see if some of the PHP experts in the core team have a good suggestion here :)

@gnat42
Copy link
Contributor Author

gnat42 commented Oct 13, 2022

Hey @wouterj yeah, that's precisely the issue. I wondered the same thing about the Throwable vs Exception. Glad for someone else to weigh in on this. One thing I'd note is that with my patch, I get what I would consider 'expected' behaviour. I get the normal symfony exception page with the stack trace etc

@carsonbot carsonbot changed the title Fix php finally executing before handleThrowable() [HttpKernel] Fix php finally executing before handleThrowable() Oct 14, 2022
@nicolas-grekas
Copy link
Member

Should be fixed by #47857

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

Successfully merging this pull request may close these issues.

4 participants