Align highlight_string|file with HTML standard and modern browsers #11913

kamil-tekiela · 2023-08-08T17:28:10Z

This PR fixes:

Instead of   output \n
Instead of   output
Instead of <code> use <pre>
Instead of  use <code>
Remove the first enclosing <code> (old )
Remove the enclosing newlines

nielsdos · 2023-08-08T17:38:15Z

I think this can't target stable branches due to the BC break, both for the internal function exposed as ZEND_API and also for the userland behaviour.

kamil-tekiela · 2023-08-08T17:40:31Z

According to the documentation the HTML markup can change at any time. https://www.php.net/manual/en/function.highlight-string.php#refsect1-function.highlight-string-notes

The HTML markup generated is subject to change.

So I think it's ok to go into minor. There should be no noticeable change in the visual output. The only difference is that the output is now block instead of inline.

nielsdos · 2023-08-08T17:41:28Z

Ah ok, wasn't aware of the note.

thg2k · 2023-08-08T18:39:18Z

I see no reason to target a stable branch with something that hasn't been touched in decades. What's wrong with 8.3?

kamil-tekiela · 2023-08-08T18:41:13Z

I see no reason to target a stable branch with something that hasn't been touched in decades. What's wrong with 8.3?

Because technically it's a bug fix. People did complain some time ago that the output of this when displayed in the browser, cannot be cleanly copied. This isn't just a stylistic change, but rather meant to fix the bug.

nielsdos · 2023-08-08T19:56:09Z

I have a quick question: why is \t translated to four spaces instead of just a tab? If we're wrapping it in a pre anyway, we might as well use a tab, or am I forgetting something?

kamil-tekiela · 2023-08-08T20:10:08Z

I have a quick question: why is \t translated to four spaces instead of just a tab? If we're wrapping it in a pre anyway, we might as well use a tab, or am I forgetting something?

Excellent question. I thought of doing the same, but I noticed that we used 4 NBSPs before. If I changed it now to a proper tab, it would change the output, because the tab is a wider character than 4 NBSP.

Test it yourself.

<pre>
&#09;HTML Tab
&nbsp;&nbsp;&nbsp;&nbsp;Four NBSPs
<?="    "?>Four spaces
<?="\t"?>Tab
</pre>

TimWolla · 2023-08-09T07:26:49Z

The size of a tab in HTML is configurable using https://developer.mozilla.org/en-US/docs/Web/CSS/tab-size

TimWolla

I am not sure if using a separate <code> tag per line is actually more correct than the current version. MDN says:

To represent multiple lines of code, wrap the <code> element within a <pre> element.

Which is phrased as a single <code> element.

Girgias

AFAIK, the proper way is to do <pre><code>code goes here where highlighting works with span elements</code></pre>

Also this can't change in a stable release, I think this is fine for 8.3 if RMs agree @bukka @ericmann

Girgias · 2023-08-09T12:38:11Z

Zend/zend_highlight.c

-				zend_printf("</span>");
+				zend_printf("</code>");


Those should still be span elements

I guess you are right. I checked how it works on Stack Overflow and they do it exactly like this. I changed it now.

kamil-tekiela · 2023-08-09T14:39:58Z

Also this can't change in a stable release

But why not? The documentation clearly states that the HTML cannot be relied upon. So as long as the actual visual output isn't changed, there should be no issue. Of course, if RMs do not agree, then let's do it in 8.3.

kamil-tekiela · 2023-08-09T14:41:58Z

I am not sure if using a separate <code> tag per line is actually more correct than the current version. MDN says:

To represent multiple lines of code, wrap the <code> element within a \<pre\> element.

Which is phrased as a single <code> element.

Correct, but then it says:

The <code> element by itself only represents a single phrase of code or line of code.

Girgias · 2023-08-09T14:48:23Z

Also this can't change in a stable release

But why not? The documentation clearly states that the HTML cannot be relied upon. So as long as the actual visual output isn't changed, there should be no issue. Of course, if RMs do not agree, then let's do it in 8.3.

The documentation is not the spec, the documentation reflects, and always will, the behaviour of the engine. So the argument "the documentation says something" does not hold up. Because if it did, dynamics properties should/could have been removed whenever, as it wasn't documented until they were deprecated.

And yes the output should not be relied upon, but changing this in a patch version makes no sense. We already have people complain about tests breaking to output changes when fixing actual bugs, so I really don't want to hear this for something that is just a cosmetic change.

Girgias · 2023-08-09T14:52:14Z

I am not sure if using a separate <code> tag per line is actually more correct than the current version. MDN says:

To represent multiple lines of code, wrap the <code> element within a \<pre\> element.

Which is phrased as a single <code> element.

Correct, but then it says:

The <code> element by itself only represents a single phrase of code or line of code.

Nothing in the living spec document seems to indicate this being valid markup: https://html.spec.whatwg.org/multipage/text-level-semantics.html#the-code-element and https://html.spec.whatwg.org/multipage/grouping-content.html#the-pre-element

bukka · 2023-08-11T10:23:08Z

I agree that this should not go to stable branches. I'm fine with 8.3 though.

kamil-tekiela · 2023-08-11T19:54:39Z

I rebased it against master, since this is what everyone agrees should be the target branch.

TimWolla

This LGTM now.

Girgias

Just add an entry into UPGRADING before merging (or after) about the format change.

Girgias · 2023-08-12T13:58:16Z

Zend/zend_highlight.c

 		case '\t':
-			ZEND_PUTS("&nbsp;&nbsp;&nbsp;&nbsp;");
+			ZEND_PUTS("    ");


Not sure if I'm a fan of replacing tabs by 4 spaces, but that's by the by.

Closes phpGH-11913

PHP 8.3 changed the details of highlighting PHP code[1], basically like we did[2] at roughly the same time. However, both don't work well together, so we avoid that. [1] <php/php-src#11913> [2] <php#79>

PHP 8.3 changed the details of highlighting PHP code[1], basically like we did[2] at roughly the same time. However, both don't work well together, so we avoid that. [1] <php/php-src#11913> [2] <#79>

kamil-tekiela force-pushed the fix-highlight_string branch from 9924c50 to 88fc877 Compare August 8, 2023 19:49

TimWolla reviewed Aug 9, 2023

View reviewed changes

Girgias requested changes Aug 9, 2023

View reviewed changes

Girgias added Waiting on RM Extension: standard labels Aug 9, 2023

Girgias mentioned this pull request Aug 9, 2023

Clarification required on ideal markup for multiline code in <pre> tag. whatwg/html#9596

Closed

Align highlight_string|file with HTML standard and modern browsers

8343be4

kamil-tekiela changed the base branch from PHP-8.1 to master August 11, 2023 19:52

kamil-tekiela requested review from iluuu1994 and bukka as code owners August 11, 2023 19:52

kamil-tekiela force-pushed the fix-highlight_string branch from 88fc877 to 8343be4 Compare August 11, 2023 19:53

github-actions bot added Category: Engine Extension: phar SAPI: cgi SAPI: cli labels Aug 11, 2023

kamil-tekiela marked this pull request as draft August 11, 2023 19:59

Revert to use again

950b893

kamil-tekiela force-pushed the fix-highlight_string branch from 9f2fd55 to 950b893 Compare August 11, 2023 22:30

kamil-tekiela marked this pull request as ready for review August 11, 2023 23:23

TimWolla approved these changes Aug 12, 2023

View reviewed changes

Girgias approved these changes Aug 12, 2023

View reviewed changes

kamil-tekiela closed this in f907a00 Aug 12, 2023

ju1ius pushed a commit to ju1ius/php-src that referenced this pull request Aug 15, 2023

Align highlight_string|file with HTML standard and modern browsers

674a1cc

Closes phpGH-11913

jorgsowa pushed a commit to jorgsowa/php-src that referenced this pull request Aug 16, 2023

Align highlight_string|file with HTML standard and modern browsers

4f1812c

Closes phpGH-11913

cmb69 mentioned this pull request Oct 5, 2024

Fix PHP highlighting for PHP ≥ 8.3 php/phd#156

Merged

Align highlight_string|file with HTML standard and modern browsers #11913

Align highlight_string|file with HTML standard and modern browsers #11913

Uh oh!

Conversation

kamil-tekiela commented Aug 8, 2023

Uh oh!

nielsdos commented Aug 8, 2023

Uh oh!

kamil-tekiela commented Aug 8, 2023

Uh oh!

nielsdos commented Aug 8, 2023

Uh oh!

thg2k commented Aug 8, 2023

Uh oh!

kamil-tekiela commented Aug 8, 2023

Uh oh!

nielsdos commented Aug 8, 2023

Uh oh!

kamil-tekiela commented Aug 8, 2023

Uh oh!

TimWolla commented Aug 9, 2023

Uh oh!

TimWolla left a comment

Choose a reason for hiding this comment

Uh oh!

Girgias left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Girgias Aug 9, 2023

Choose a reason for hiding this comment

Uh oh!

kamil-tekiela Aug 12, 2023

Choose a reason for hiding this comment

Uh oh!

kamil-tekiela commented Aug 9, 2023

Uh oh!

kamil-tekiela commented Aug 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Girgias commented Aug 9, 2023

Uh oh!

Girgias commented Aug 9, 2023

Uh oh!

bukka commented Aug 11, 2023

Uh oh!

kamil-tekiela commented Aug 11, 2023

Uh oh!

TimWolla left a comment

Choose a reason for hiding this comment

Uh oh!

Girgias left a comment

Choose a reason for hiding this comment

Uh oh!

Girgias Aug 12, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Girgias left a comment •

edited

Loading

kamil-tekiela commented Aug 9, 2023 •

edited

Loading