[ErrorHandler] Improve fileLinkFormat handling #50734
Conversation
| Q | A |
|---|---|
| Branch? | 6.4 |
| Bug fix? | no |
| New feature? | yes |
| Deprecations? | yes |
| Tickets | Fix #50619 |
| License | MIT |
| Doc PR |
- 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
|
Hey! I see that this is your first PR. That is great! Welcome! Symfony has a contribution 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 to
81c102d
Compare
There was a problem hiding this comment.
Using the Symfony\Component\HttpKernel\Debug\FileLinkFormatter in Symfony\Component\ErrorHandler\ErrorRenderer\HtmlErrorRenderer is a good idea, but the 2 classes are not in the same composer packages.
I think the FileLinkFormatter should be moved to symfony/error-handler: this package is already required by symfony/http-kernel, but not in the other direction.
This can be done in a backward-compatible way by replacing the class with:
namespace Symfony\Component\HttpKernel\Debug;
use Symfony\Component\ErrorHandler\ErrorRenderer\FileLinkFormatter as NewFileLinkFormatter;
/** @deprecated since 6.4, use Symfony\Component\ErrorHandler\ErrorRenderer\FileLinkFormatter */
class FileLinkFormatter extends 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
Outdated
Show resolved
Hide resolved
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 ($fileLinkFormat instanceof FileLinkFormatter) {
$this->fileLinkFormat = $fileLinkFormat;
} else {
$this->fileLinkFormat = class_exists(FileLinkFormatter::class) ? new FileLinkFormatter() : $fileLinkFormat;
}What do you think @GromNaN?
That would be welcome! |
29f3941 to
32346bf
Compare
32346bf to
3cc9728
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 to
510b77b
Compare
|
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 | MIT Leftover from #50734 Commits ------- 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 | MIT Follow up of #50734 Commits ------- 842e6ab [ErrorHandler] Fix expected missing return types
|
Great, thanks! |