enh(php) detect newer more flexible HEREdoc syntax #2658
Conversation
|
#2651 Resolves this issue |
src/languages/php.js
Outdated
| begin: /<<<[ \t]*(?<marker>\w+)\n(?:.*\n)*?^[ \t]*\k<marker>\b/, | ||
| returnBegin: true, | ||
| contains: [ | ||
| { begin: /<<<[ \t]*/ }, |
There was a problem hiding this comment.
I don't think se need any of this... move the <<< into the BEGIN match... we shouldn't only need a simple being/end match here (inside the END_SAME_AS_BEGIN) helper, no?
There was a problem hiding this comment.
Ie, we don't need to find the end first...
There was a problem hiding this comment.
right, I missed that copying from ruby.js
src/languages/php.js
Outdated
| var SINGLE_QUOTED = hljs.inherit( | ||
| hljs.APOS_STRING_MODE, | ||
| { | ||
| illegal: null, | ||
| variants: [{ | ||
| begin: 'b\'', end: '\'' | ||
| }] | ||
| } | ||
| ); | ||
| var DOUBLE_QUOTED = hljs.inherit( | ||
| hljs.QUOTE_STRING_MODE, | ||
| { | ||
| illegal: null, | ||
| variants: [{ | ||
| begin: 'b"', end: '"' | ||
| }], | ||
| contains: [hljs.BACKSLASH_ESCAPE, SUBST] | ||
| } | ||
| ); |
There was a problem hiding this comment.
Pretty sure this is wrong and not the equivalent of what we were doing before, it's also MUCH harder to read. (it's unclear that there are two branches). Pretty sure variants needs to spell out ALL the variants. You're also making every variant null, which is also incorrect, etc...
Variants are a complete list not "in addition to"....
I'd make another attempt here or just revert this to what we had before.
There was a problem hiding this comment.
Maybe I misunderstood thinking the variants of the main will result in the main + expansion for each variant, isn't it? It was working as expected testing it, so how? I wanted to reflect the fact that each string (single and double) has its own "binary" equivalent for PHP6 compatibility...
There was a problem hiding this comment.
Maybe I misunderstood thinking the variants of the main will result in the main + expansion for each variant, isn't it?
No. Variants are exclusive (they merge over-top of the "defaults" from the top).
I wanted to reflect the fact that each string (single and double) has its own "binary" equivalent for PHP6 compatibility...
I think we can just do this with comments the way this was prior. Otherwise you're repeating a lot of the string bits over and over for no real big win...
There was a problem hiding this comment.
Ok so I noticed when retesting my code that binary strings were broken so I fixed it. I seems the variants where not expanded recursively, a bug maybe ? (Prior the substitution was not used in binary strings, not a big win because it surely affects nobody as it is a backward compatiblity residue but seems clearer...)
There was a problem hiding this comment.
Ok so I noticed when retesting my code that binary strings were broken so I fixed it. I seems the variants where not expanded recursively, a bug maybe ?
The problem before was variants are not "in addition to". Whether variants can nest inside other variants I'm not sure.
There was a problem hiding this comment.
Prior the substitution was not used in binary strings
Nice fix. Should mention that in CHANGES.md also.
…/highlight.js into php_flexible_heredoc_syntax_rfc
|
Looks good, lets update the CHANGES.md file and I think I can merge. |
joshgoebel
changed the title
src/languages/php.js
Outdated
| contains: hljs.QUOTE_STRING_MODE.contains.concat(SUBST), | ||
| }); | ||
| var HEREDOC = { | ||
| begin: /<<<[ \t]*(?<marker>\w+)\n(?:.*\n)*?^[ \t]*\k<marker>\b/, |
There was a problem hiding this comment.
Did we not discuss simplifying all this?
There was a problem hiding this comment.
yes i moved the "<<<" into the end_same_as_begin, it seems good now, the regex is clear with the named group marker, ready yo be merged ?
There was a problem hiding this comment.
My point is why do we need lines 40/41/42 at all? ALL we really need here is just the END_SAME_AS_BEGIN rule without any other rules, no?
There was a problem hiding this comment.
right, I missed that copying from ruby.js, fixed!
…/highlight.js into php_flexible_heredoc_syntax_rfc
Allows the use of the new indented heredoc : https://wiki.php.net/rfc/flexible_heredoc_nowdoc_syntaxes