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.

Sign up for GitHub

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

Jump to bottom

enh(php) detect newer more flexible HEREdoc syntax #2658

Merged
merged 12 commits into from
Aug 31, 2020
Merged

enh(php) detect newer more flexible HEREdoc syntax #2658

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
fe8cdc0
share subst with double quoted string
eytienne Aug 11, 2020
a410d43
separate tests along purpose + support of indented heredoc + heredoc …
eytienne Aug 18, 2020
c7417a0
Merge branch 'master' into php_flexible_heredoc_syntax_rfc
eytienne Aug 18, 2020
500a638
fix unclear beginning
eytienne Aug 18, 2020
b4c62bc
Merge branch 'php_flexible_heredoc_syntax_rfc' of github.com:eytienne…
eytienne Aug 18, 2020
d07c1af
fix binary string was broken + concat to main for subst
eytienne Aug 19, 2020
e3eb55a
Merge branch 'master' into php_flexible_heredoc_syntax_rfc
eytienne Aug 21, 2020
043dab9
Update CHANGES.md
eytienne Aug 21, 2020
d400139
Update CHANGES.md
eytienne Aug 21, 2020
355e1cf
Merge branch 'master' into php_flexible_heredoc_syntax_rfc
eytienne Aug 26, 2020
6ae53c7
removes redundancy
eytienne Aug 29, 2020
99449d8
Merge branch 'php_flexible_heredoc_syntax_rfc' of github.com:eytienne…
eytienne Aug 29, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
63 changes: 41 additions & 22 deletions src/languages/php.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ Website: https://www.php.net
Category: common
*/

/** @returns {LanguageDetail} */
export default function(hljs) {
var VARIABLE = {
begin: '\\$+[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*'
Expand All @@ -18,18 +19,50 @@ export default function(hljs) {
{ begin: /\?>/ } // end php tag
]
};
var SUBST ={
className: 'subst',
variants: [
{begin: /\$\w+/},
{begin: /\{\$/, end: /\}/}
]
};
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]
}
);
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@eytienne eytienne Aug 18, 2020 •
edited

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@eytienne eytienne Aug 19, 2020 •
edited

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Prior the substitution was not used in binary strings

Nice fix. Should mention that in CHANGES.md also.

var HEREDOC = {
begin: /<<<[ \t]*(?<marker>\w+)\n(?:.*\n)*?^[ \t]*\k<marker>\b/,
Copy link
Member

Choose a reason for hiding this comment

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

Did we not discuss simplifying all this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 ?

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right, I missed that copying from ruby.js, fixed!

returnBegin: true,
contains: [
{ begin: /<<<[ \t]*/ },
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

Ie, we don't need to find the end first...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right, I missed that copying from ruby.js

hljs.END_SAME_AS_BEGIN({
begin: /(\w+)/, end: /[ \t]*(\w+)\b/,
contains: [hljs.BACKSLASH_ESCAPE, SUBST],
}),
]
};
var STRING = {
className: 'string',
contains: [hljs.BACKSLASH_ESCAPE, PREPROCESSOR],
variants: [
{
begin: 'b"', end: '"'
},
{
begin: 'b\'', end: '\''
},
hljs.inherit(hljs.APOS_STRING_MODE, {illegal: null}),
hljs.inherit(hljs.QUOTE_STRING_MODE, {illegal: null})
SINGLE_QUOTED,
DOUBLE_QUOTED,
HEREDOC
]
};
var NUMBER = {variants: [hljs.BINARY_NUMBER_MODE, hljs.C_NUMBER_MODE]};
Expand Down Expand Up @@ -87,20 +120,6 @@ export default function(hljs) {
keywords: '__halt_compiler'
}
),
{
className: 'string',
begin: /<<<['"]?\w+['"]?$/, end: /^\w+;?$/,
contains: [
hljs.BACKSLASH_ESCAPE,
{
className: 'subst',
variants: [
{begin: /\$\w+/},
{begin: /\{\$/, end: /\}/}
]
}
]
},
PREPROCESSOR,
{
className: 'keyword', begin: /\$this\b/
Expand Down
14 changes: 0 additions & 14 deletions test/markup/php/heredoc.expect.txt
View file
Open in desktop

This file was deleted.

14 changes: 0 additions & 14 deletions test/markup/php/heredoc.txt
View file
Open in desktop

This file was deleted.

23 changes: 23 additions & 0 deletions test/markup/php/strings.expect.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
<span class="hljs-meta">&lt;?php</span>

<span class="hljs-comment">// variable substitution</span>

<span class="hljs-keyword">echo</span> (<span class="hljs-string">&#x27;Hello {$person-&gt;name}! Welcome to $company!&#x27;</span>);

<span class="hljs-keyword">echo</span> (<span class="hljs-string">&quot;Hello <span class="hljs-subst">{$person-&gt;name}</span>! Welcome to <span class="hljs-subst">$company</span>!&quot;</span>);

<span class="hljs-keyword">echo</span> (<span class="hljs-string">&lt;&lt;&lt;MSG
Hello <span class="hljs-subst">{$person-&gt;name}</span>! Welcome to <span class="hljs-subst">$company</span>!
MSG</span>);

<span class="hljs-comment">// heredoc syntax</span>

var_dump(<span class="hljs-string">&lt;&lt;&lt;SQL
SELECT *
FROM table
SQL</span>);

var_dump(<span class="hljs-string">&lt;&lt;&lt;SQL
SELECT *
FROM table
SQL</span>);
23 changes: 23 additions & 0 deletions test/markup/php/strings.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
<?php

// variable substitution

echo ('Hello {$person->name}! Welcome to $company!');

echo ("Hello {$person->name}! Welcome to $company!");

echo (<<<MSG
Hello {$person->name}! Welcome to $company!
MSG);

// heredoc syntax

var_dump(<<<SQL
SELECT *
FROM table
SQL);

var_dump(<<<SQL
SELECT *
FROM table
SQL);