Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
use try-finally when possible#15949
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@nicolas-grekas I assume this hack also works with finally, i.e. fatal errors are skipping it. I tried to test it as described in#14342. And it had the same result. So this code change didn't change the outcome of the exception page. But it was kinda strange anyway as there was a second error below the real one:
FatalErrorException in TraceableEventDispatcher.php line 357:Error: Cannot use object of type Symfony\Component\EventDispatcher\Debug\WrappedListener as arrayin vendor\symfony\symfony\src\Symfony\Component\EventDispatcher\Debug\TraceableEventDispatcher.php at line 357fabpot commentedSep 28, 2015
Thank you@Tobion. |
This PR was merged into the 3.0-dev branch.Discussion----------use try-finally when possible| Q | A| ------------- | ---| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | I hope| Fixed tickets | n/a| License | MIT| Doc PR | n/aFound those with regex `catch \(\\Exception[^\}]+throw \$`Commits-------49edef2 use try-finally when possible
lyrixx commentedSep 28, 2015
@fabpot finally does not work on PHP 5.5. refs:https://bugs.php.net/bug.php?id=67047 ihmo, symfony should never use finally, except if we bump the requirements on php 5.6 |
nicolas-grekas commentedSep 28, 2015 via email
Well, finally works, but autoloading does not work in a finally blockbefore 5.6. I reviewed the current patch and it looks safe for 5.5 but wemust have this issue in mind. |
lyrixx commentedSep 28, 2015
that's why I think it's saver to recall: "never use it". Anyway, I may be too "strict" and may be it's better to be allow it when it's really safe. Don't know ;) But again, all deciders should be aware of this bug to not merge some PR that trigger autoload. |
Tobion commentedSep 28, 2015
@lyrixx thanks for pointing out. But as using autoloading in finally is an edge case, it should not prevent us using finally. It's an uncommon case because usually you do something in finally that you already accessed before (a variable, class or whatever to revert state). So autoloading there should not happen. |
lyrixx commentedSep 28, 2015
@Tobion I totally agree with you ;) In theses case there is no issue. But image this piece of code: ;) |
This PR was merged into the 3.0-dev branch.Discussion----------[DI] use try-finally for container| Q | A| ------------- | ---| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | n/a| License | MIT| Doc PR | n/aContinuation of#15949Commits-------1ab7316 [DI] use try-finally for container
This PR was merged into the 2.7 branch.Discussion----------Catch \Throwable| Q | A| ------------- | ---| Branch? | 2.7, 2.8, 3.0| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | Yes| Fixed tickets | n/a| License | MIT| Doc PR | n/aRelated#18765,#15949Depends on#18812Commits-------103526b Catch \Throwable
This PR was merged into the 2.8 branch.Discussion----------Catch \Throwable| Q | A| ------------- | ---| Branch? | 2.8, 3.0| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | Mostly!| Fixed tickets | n/a| License | MIT| Doc PR | n/aThe first commit is based on#15949Depends on#18813,#18812----I'm new to symfony, so I'm not sure where are all the places where it makes sense to actually catch the throwable and where not. I added most places that seemed logical and when I wasn't sure, I added it anyway. I'm hoping you guys (and girls?) can point out the places where the catch should not be added, I'll fix it and then I can create several PR's for the older branches. A lot of this IMHO should go also to 3.0.Commits-------de671f4 Catch \Throwable
This PR was merged into the 2.8 branch.Discussion----------Catch \Throwable| Q | A| ------------- | ---| Branch? | 2.8, 3.0| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | Mostly!| Fixed tickets | n/a| License | MIT| Doc PR | n/aThe first commit is based onsymfony/symfony#15949Depends onsymfony/symfony#18813,symfony/symfony#18812----I'm new to symfony, so I'm not sure where are all the places where it makes sense to actually catch the throwable and where not. I added most places that seemed logical and when I wasn't sure, I added it anyway. I'm hoping you guys (and girls?) can point out the places where the catch should not be added, I'll fix it and then I can create several PR's for the older branches. A lot of this IMHO should go also to 3.0.Commits-------de671f4 Catch \Throwable
This PR was squashed before being merged into the 2.3 branch (closes#18812).Discussion----------Catch \Throwable| Q | A| ------------- | ---| Branch? | 2.3, 2.7, 2.8, 3.0| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | Yes| Fixed tickets | n/a| License | MIT| Doc PR | n/aRelated#18765,#15949Commits-------893cf00 Catch \Throwable
Found those with regex
catch \(\\Exception[^\}]+throw \$