Skip to content
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

Merged
merged 5 commits into from Mar 19, 2023
Merged

PHP NOWDOC Support #3679

merged 5 commits into from Mar 19, 2023

Conversation

doiftrue
Copy link
Contributor

@doiftrue doiftrue commented Dec 17, 2022

Support NOWDOC syntax for php.js.

NOTE: NOWDOC don't parse php variables inside the string.

HEREDOC improvements: quote syntax support <<<"END".

Checklist

  • Added markup tests.
  • Updated the changelog at CHANGES.md

@@ -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/,
Copy link
Member

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.

Copy link
Contributor Author

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.

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/,
Copy link
Member

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?

Copy link
Contributor Author

@doiftrue doiftrue Dec 18, 2022

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

Copy link
Member

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?

@@ -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/,
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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...

Copy link
Contributor Author

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?

Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And even more we have one more problem here
image

Copy link
Contributor Author

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(); },
  };

Copy link
Member

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.

Copy link
Contributor Author

@doiftrue doiftrue Dec 18, 2022

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.

Copy link
Member

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.

Copy link
Member

@joshgoebel joshgoebel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Thanks!

@joshgoebel joshgoebel merged commit b33384d into highlightjs:main Mar 19, 2023
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants