-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
PHP NOWDOC Support #3679
PHP NOWDOC Support #3679
Conversation
src/languages/php.js
Outdated
@@ -56,14 +56,19 @@ export default function(hljs) { | |||
end: /[ \t]*(\w+)\b/, | |||
contains: hljs.QUOTE_STRING_MODE.contains.concat(SUBST), | |||
}); | |||
const NOWDOC = hljs.END_SAME_AS_BEGIN({ | |||
begin: /<<<[ \t']*(\w+)'?\n/, |
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.
The '
here doesn't look correct. I think you want something like:
(\w+|'\w+')
# unquoted or (|) quoted
What you have now would match ''''''''''WORD
which is not correct.
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.
Good catch! Fixed! Made nowdoc more strict.
Also, quote (") wrapper support added for HEREDOC >>> "END".
PHP allows write <<<END
OR <<<"END"
for heredoc. Now <<<"END"
supported and tests improved.
…for HEREDOC >>> "EOL".
end: /[ \t]*(\w+)\b/, | ||
contains: hljs.QUOTE_STRING_MODE.contains.concat(SUBST), | ||
}); | ||
const NOWDOC = hljs.END_SAME_AS_BEGIN({ | ||
begin: /<<<[ \t']*(\w+)'?\n/, | ||
begin: /<<<[ \t]*'(\w+)'\n/, |
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.
It must always be quoted?
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.
Yes. If not - it's a HEREDOC not NOWDOC
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.
But the end of now doc doesn't have to be quoted?
src/languages/php.js
Outdated
@@ -52,12 +52,12 @@ export default function(hljs) { | |||
contains: hljs.QUOTE_STRING_MODE.contains.concat(SUBST), | |||
}); | |||
const HEREDOC = hljs.END_SAME_AS_BEGIN({ | |||
begin: /<<<[ \t]*(\w+)\n/, | |||
begin: /<<<[ \t]*"?(\w+)"?\n/, |
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.
This allows:
"BOO
BOO"
You need to use an OR (like I showed you before) to make sure they are balanced.
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.
"
itself must not be included in match. For now I don't know how to do that. (\w+|"\w+")
this is not work.
I don't think it's a problem. It's just highlighter but not PHP code parser.
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.
<<<[ \t]*(?:(\w+)|"(\w+)")
don't works as well because hljs.END_SAME_AS_BEGIN works with first group only...
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.
Any ideas how to solve this?
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.
Make the outside group capturing and the two inside ones non-capturing - though technically you don't need the inside groups at all the |
and outside group should be fine by themselves.
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.
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.
The only solution I found - is rewrite hljs.END_SAME_AS_BEGIN
and then (?:(\w+)|"(\w+)")
works good:
const HEREDOC = {
begin: /<<<[ \t]*(?:(\w+)|"(\w+)")\n/,
end: /[ \t]*(\w+)\b/,
contains: hljs.QUOTE_STRING_MODE.contains.concat(SUBST),
'on:begin': (m, resp) => { resp.data._beginMatch = m[1] || m[2]; },
'on:end': (m, resp) => { if (resp.data._beginMatch !== m[1]) resp.ignoreMatch(); },
};
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.
Yeah, I was just about to mention it's just JS under the covers and you could tweak it like that... :-) Looks good enough to me.
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.
But I think it's better to leave begin: /<<<[ \t]*"?(\w+)"?\n/,
.
That's okay about "BOO
& BOO"
. Because we, in other hand, don't check the full workability of the HEREdoc construct, which implies that the indentation of the text will be exactly the same as the indentation of last END.
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.
When it's easy to avoid false positives with a strong regex, we go that route... which is this case means matching quoted OR unquoted, not half quoted.
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.
Looks good! Thanks!
Support NOWDOC syntax for php.js.
NOTE: NOWDOC don't parse php variables inside the string.
HEREDOC improvements: quote syntax support
<<<"END"
.Checklist
CHANGES.md