Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[ErrorHandler] Improve fileLinkFormat handling#50734
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
carsonbot commentedJun 21, 2023
Hey! I see that this is your first PR. That is great! Welcome! Symfony has acontribution guide which I suggest you to read. In short:
Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change. When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor! I am going to sit back now and wait for the reviews. Cheers! Carsonbot |
58c831b
to81c102d
Compare
GromNaN left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
Using theSymfony\Component\HttpKernel\Debug\FileLinkFormatter
inSymfony\Component\ErrorHandler\ErrorRenderer\HtmlErrorRenderer
is a good idea, but the 2 classes are not in the same composer packages.
I think theFileLinkFormatter
should be moved tosymfony/error-handler
: this package is already required bysymfony/http-kernel
, but not in the other direction.
This can be done in a backward-compatible way by replacing the class with:
namespaceSymfony\Component\HttpKernel\Debug;useSymfony\Component\ErrorHandler\ErrorRenderer\FileLinkFormatterasNewFileLinkFormatter;/** @deprecated since 6.4, use Symfony\Component\ErrorHandler\ErrorRenderer\FileLinkFormatter */class FileLinkFormatterextends NewFileLinkFormatter{// Inherit all the methods from the parent}
I'll just ask @symfony/mergers opinion before you can start editing if you wish.
src/Symfony/Component/ErrorHandler/ErrorRenderer/HtmlErrorRenderer.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
nlemoine commentedJun 23, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
That would be great! My original issue about was actually aiming to consolidate file link formatting and make it more consistent across components. Some business logic, that's already contained in
Probably all those statements could be replaced with something like (long version): if ($fileLinkFormatinstanceof FileLinkFormatter) {$this->fileLinkFormat =$fileLinkFormat;}else {$this->fileLinkFormat =class_exists(FileLinkFormatter::class) ?newFileLinkFormatter() :$fileLinkFormat;} What do you think@GromNaN?
That would be welcome! |
29f3941
to32346bf
Compare32346bf
to3cc9728
Compare- Avoid repeating file link format guessing (logic is already in FileLinkFormatter class)- Always set a fileLinkFormat to a FileLinkFormatter object to handle path mappings properly
3cc9728
to510b77b
CompareThere 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.
I updated all references to the deprecated namespace.
This should be good to do (despite tests)
Thank you@nlemoine. |
…fancyweb)This PR was merged into the 6.4 branch.Discussion----------[ErrorHandler] Fix file link format call in trace view| Q | A| ------------- | ---| Branch? | 6.4| Bug fix? | yes| New feature? | no| Deprecations? | no| Tickets | no| License | MITLeftover from#50734Commits-------c2f01c2 [ErrorHandler] Fix file link format call in trace view
…dre-daubois)This PR was merged into the 6.4 branch.Discussion----------[ErrorHandler] Fix expected missing return types| Q | A| ------------- | ---| Branch? | 6.4| Bug fix? | yes| New feature? | no| Deprecations? | no| Tickets | -| License | MITFollow up of#50734Commits-------842e6ab [ErrorHandler] Fix expected missing return types
Great, thanks! |
Uh oh!
There was an error while loading.Please reload this page.