[Debug] Remove GLOBALS from exception context to avoid endless recursion #20519
Conversation
|
This piece of code changed in 3.2, are you sure the issue is on <= 3.1? |
|
I could repro with 3.1.6, not sure about 3.2 though, but in general I find it surprising/disappointing that this whole code reinvents exception serialization instead of passing on the raw exception object to the logger context as defined in PSR-3, which would have allowed monolog to serialize it well in this case. Obviously I am a bit biased but still. |
|
So, check 3.2, which does just that :) |
|
OK I can confirm that on symfony master the bug isn't there anymore, that's great! I don't know if we should merge this fix anyway for lower versions or not. It's kind of an edge case that something bad happens on the global scope these days, so up to you. |
|
I'm still in favor of fixing such tricky edge case (especially when we have the fix), as infinite loops in the error handling makes debugging very hard |
|
Looking a few lines above, I'd say let's unset GLOBALS all the time, no matter which PHP version is in use. The fix should be just removing the PHP version check from this line. |
|
Ah I thought 2.7 was EOL sorry :) I'll rebase, and you rather do the unset above there without any condition around it is that right? |
|
I'd keep the |
|
OK thanks, I'm asking because I have no idea what the rest is about and don't really have time to dig in :) |
|
Alright let me know if there's anything else I can do here |
|
Thank you @Seldaek. |
…less recursion (Seldaek) This PR was merged into the 2.7 branch. Discussion ---------- [Debug] Remove GLOBALS from exception context to avoid endless recursion | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | License | MIT This fixes a endless recursion as seen in felixfbecker/php-language-server#137 - it only happens if you trigger an error in the global context so it's kinda hard to write a test for, but hopefully the fix can be accepted. Commits ------- 1bb95ac [Debug] Remove GLOBALS from exception context to avoid endless recursion
|
FYI http://symfony.com/roadmap?version=2.7#checker |
|
This causes BC break for us while upgrading from 3.1.6 to 3.1.7 as we are wrapping a legacy codebase highly coupled to |
|
@antograssiot should be easy to add a monolog processor that adds $GLOBALS (or a subset) to the context or extra data of log records if you really want.. I think this stuff is way better done in the app than the framework as it depends on application specific knowledge. |
|
@Seldaek what @antograssiot said is more about the issue introduced by the unrecoverable and unconfigurable deletion of $GLOBALS. Our case (we're teammates fyi) is that we have a wrapped legacy system which is "called" by the symfony app when the request can't be processed by symfony (exceptions thrown by includes of legacy files are catched and handled...). It is kind of a classical, clean and smooth progressive migration pattern. The legacy system relying on $GLOBALS is fully broken: OK, we all agree about how dramatic it is but hey, sometimes reality is just as it is ;). |
|
How does this relate to your app? I mean, did you do some integration with the |
|
I created a repository to illustrate exactly our use case : https://github.com/philippecarle/symfony-globals Here is a brief explanation of our application behavior : the app.php tries to handle the request as it uses to, but in case of thrown NotFoundHttpException, the legacyhandler service tries to resolve the path in the /legacy folder and handle either the response or the exception. I put a legacy/test/test.php file to illustrate what happens, dumping $GLOBALS once, triggering an error (in my case an undeclared variable) and dumping the $GLOBALS again. In 3.1.6 it was ok, but the 3.1.7 just cleans the globals. Our legacy code intensively relies on globals, so it makes us being stuck in 3.1.6. We may find a not-too-tricky way to disable errors handling for the legacy code, but it would prevent us from using the debug section of the profiler, which is very useful for us in our everyday work of optimization of the legacy code. Any ideas / suggestions ? |
This fixes a endless recursion as seen in felixfbecker/php-language-server#137 - it only happens if you trigger an error in the global context so it's kinda hard to write a test for, but hopefully the fix can be accepted.