[TwigBundle] Give some love to exception pages #20525
Conversation
| @@ -216,6 +215,27 @@ public function formatFileFromText($text) | |||
| } | |||
|
|
|||
| /** | |||
| * @internal | |||
| */ | |||
| public function formatLogMessage($message, $context) | |||
There was a problem hiding this comment.
second argument should be typehinted as array IMO
nicolas-grekas
force-pushed
the
fix-exception-page
branch
2 times, most recently
from
7d249b0 to
10b9db2
Compare
|
Are they opened by default? If so, can you add a limit of maybe 5 or 10 logs? I've had pages where there are 100+ logs and that will make it annoying to find the exception. |
|
@iltar this PR is about fixing this page, ie make it work again; not about making it better. We could (and should) work on that for 3.3. But this one is a bug fix (placeholders are not replaced right now, and CSS needs a fix in the code block excerpt). |
nicolas-grekas
force-pushed
the
fix-exception-page
branch
from
10b9db2 to
d8e9b6c
Compare
|
I mis-read the code as well. 👍 |
|
Ah my bad, I only saw new code 😆 |
| @@ -151,7 +152,7 @@ public function fileExcerpt($file, $line, $srcContext = 3) | |||
| } | |||
|
|
|||
| for ($i = max($line - $srcContext, 1), $max = min($line + $srcContext, count($content)); $i <= $max; ++$i) { | |||
| $lines[] = '<li'.($i == $line ? ' class="selected"' : '').'><div class="anchor" id="line'.$i.'"></div><code>'.self::fixCodeMarkup($content[$i - 1]).'</code></li>'; | |||
| $lines[] = '<li'.($i == $line ? ' class="selected"' : '').'><a class="anchor" name="line'.$i.'"></a><code>'.self::fixCodeMarkup($content[$i - 1]).'</code></li>'; | |||
There was a problem hiding this comment.
a[name] is not valid HTML5... what about <li id="x"><code></code></li> As anchor linking will work on any ID anyway...
There was a problem hiding this comment.
There can be only one specific id on a page, yet this is called several times.
There was a problem hiding this comment.
Maybe add a static counter internally? To create something unique..
There was a problem hiding this comment.
If the target is not predictable, we're going to have a hard time linking to it...
There was a problem hiding this comment.
I see. Maybe generate a hash of file+line.. or something, and only add a unique prefix for the 2nd and upcoming instances (ie. the first instance is predictable).
There was a problem hiding this comment.
a with name works, let's not create a pile of code to solve no actual issue...
There was a problem hiding this comment.
Imo. it should produce valid HTML5 :).. but fair enough.
|
Thank you @nicolas-grekas. |
…ekas) This PR was merged into the 3.2-dev branch. Discussion ---------- [TwigBundle] Give some love to exception pages | Q | A | ------------- | --- | Branch? | 3.2 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - Before:  After:  Commits ------- d8e9b6c [TwigBundle] Give some love to exception pages
Before:
After:
