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

[RFC] allow more whitespace in simple tokens #443

Merged
merged 6 commits into from May 10, 2019

Conversation

Projects
None yet
5 participants
@fritzmg
Copy link
Contributor

commented Apr 18, 2019

I rarely use simple tokens - but when I do I always forget that the expressions must not have any white space between token, operator and vlaue, i.e. things like

{if foo == "moo"}

will not work.

Are there any objections to expanding the regex in order to allow whitespace between token, operator and value?

https://regex101.com/r/0rrdnV/2

@gmpf

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2019

Not an objection in principle, but I think we shouldn't include line breaks in that white space. If you allow this:

{if foo
==
"moo"}

users will wonder why that works but the following doesn't:

{if
foo == "moo"
}

So, IMO, \h is more suitable than \s.

@@ -584,7 +584,7 @@ function (array $matches) use ($arrData)
$evaluateExpression = function ($strExpression) use ($arrData)
{
if (!preg_match('/^([^=!<>\s]+)([=!<>]+)(.+)$/is', $strExpression, $arrMatches))
if (!preg_match('/^([^=!<>\s]+)[\s]*([=!<>]+)[\s]*(.+)$/is', $strExpression, $arrMatches))

This comment has been minimized.

Copy link
@ausi

ausi Apr 26, 2019

Member

I would change this to:

Suggested change
if (!preg_match('/^([^=!<>\s]+)[\s]*([=!<>]+)[\s]*(.+)$/is', $strExpression, $arrMatches))
if (!preg_match('/^([^=!<>\s]+)[ \t]*([=!<>]+)(.+)$/is', $strExpression, $arrMatches))

and remove the whitepace around the value in line 594:

$strValue = trim($arrMatches[3], " \t");

This comment has been minimized.

Copy link
@leofeyer

leofeyer Apr 30, 2019

Member

I would only allow spaces and not tabs.

This comment has been minimized.

Copy link
@fritzmg

fritzmg May 9, 2019

Author Contributor

I have added the proposed changes. @ausi @leofeyer can you have a look at it again?

I also tested it via the registration module.

@ausi

This comment has been minimized.

Copy link
Member

commented Apr 26, 2019

We’ve rewritten the simple tokens in contao/core-bundle#570 but I don’t remember why we didn’t allow whitespace.

Are there any objections…

IMO no, supporting whitespace should be straightforward.

@Toflar

This comment has been minimized.

Copy link
Member

commented Apr 26, 2019

Can you extend the unit tests so we can see if there are side effects?

@leofeyer

This comment has been minimized.

Copy link
Member

commented Apr 26, 2019

but I don’t remember why we didn’t allow whitespace.

Probably because Smarty does it this way and to prevent false-positives with other template languages that use a syntax like { var }? But since simple tokens are typically not used inside templates, this should not be an issue.

@fritzmg fritzmg changed the title allow more whitespace in simple tokens [RFC] allow more whitespace in simple tokens Apr 26, 2019

@leofeyer leofeyer added the feature label Apr 30, 2019

@leofeyer leofeyer added this to the 4.8.0 milestone Apr 30, 2019

@fritzmg

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2019

Btw. do we also want to allow foo == 'foo' instead of just foo == "foo"?

@@ -586,14 +586,14 @@ function (array $matches) use ($arrData)
$evaluateExpression = function ($strExpression) use ($arrData)
{
if (!preg_match('/^([^=!<>\s]+)([=!<>]+)(.+)$/s', $strExpression, $arrMatches))
if (!preg_match('/^([^=!<>\s]+)[ ]*([=!<>]+)(.+)$/s', $strExpression, $arrMatches))

This comment has been minimized.

Copy link
@leofeyer

leofeyer May 9, 2019

Member

)[ ]*( can be simplified to ) *( or even ) ?( (the latter would allow not more than one space).

This comment has been minimized.

Copy link
@ausi

ausi May 9, 2019

Member

I’d go with ) *(

{
return false;
}
$strToken = $arrMatches[1];
$strOperator = $arrMatches[2];
$strValue = $arrMatches[3];
$strValue = trim($arrMatches[3], " \t");

This comment has been minimized.

Copy link
@ausi

ausi May 9, 2019

Member
Suggested change
$strValue = trim($arrMatches[3], " \t");
$strValue = trim($arrMatches[3], " ");

This comment has been minimized.

Copy link
@leofeyer

leofeyer May 9, 2019

Member

With single quotes though:

Suggested change
$strValue = trim($arrMatches[3], " \t");
$strValue = trim($arrMatches[3], ' ');
@leofeyer

This comment has been minimized.

Copy link
Member

commented May 9, 2019

Btw. do we also want to allow foo == 'foo' instead of just foo == "foo"?

Both single and double quotes are supported already, aren't they?

@ausi

This comment has been minimized.

Copy link
Member

commented May 9, 2019

Both single and double quotes are supported already, aren't they?

Yes, they are:

"\n{if token=='foo'}\nline2\n{endif}\n",

@fritzmg

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2019

Oh, right, I missed that :)

@Toflar
Copy link
Member

left a comment

Tests are still missing...

@fritzmg

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2019

Yeah, I'll add them today.

@fritzmg

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2019

I have added tests for three cases: spaces only between variable name, operator and value, spaces with tabs and spaces with line breaks. For the latter two, the expression should never match, since we only allow spaces.

@Toflar

Toflar approved these changes May 9, 2019

@leofeyer

This comment has been minimized.

Copy link
Member

commented May 9, 2019

Actually, I don't think we should support multiple spaces. 🙈

{if foo   ==   'bar'}

What's the point of having multiple spaces? One or none should suffice.

@ausi

This comment has been minimized.

Copy link
Member

commented May 9, 2019

What's the point of having multiple spaces?

Alignment?

{if foo         == 'bar'       }A{endif}
{if veryLongFoo == 'bar2'      }B{endif}
{if shortFoo    == 'butLongBar'}C{endif}
@ausi

ausi approved these changes May 9, 2019

@leofeyer

This comment has been minimized.

Copy link
Member

commented May 10, 2019

Your example is invalid though, because you cannot have spaces before the closing bracket. 🤓 So all you could do is this:

{if foo         == 'bar'}A{endif}
{if veryLongFoo == 'bar2'}B{endif}
{if shortFoo    == 'butLongBar'}C{endif}
@fritzmg

This comment has been minimized.

Copy link
Contributor Author

commented May 10, 2019

Maybe we should make that possible then? 😁

@leofeyer

This comment has been minimized.

Copy link
Member

commented May 10, 2019

Yes, maybe we should. I find { if foo == 'bar' }A{ endif } more readable than {if foo == 'bar'}A{endif}.

@ausi

This comment has been minimized.

Copy link
Member

commented May 10, 2019

Your example is invalid though, because you cannot have spaces before the closing bracket.

Are you sure? If I read the code correctly, spaces after the value should be fine because of the trim()...

I find { if foo == 'bar' }A{ endif } more readable than {if foo == 'bar'}A{endif}.

But that change would be backwards incompatible :(

@leofeyer

This comment has been minimized.

Copy link
Member

commented May 10, 2019

Are you sure? If I read the code correctly, spaces after the value should be fine because of the trim()...

As usual, you are right. 😄 Then I guess I will adjust the unit tests and merge this PR.

@leofeyer leofeyer force-pushed the fritzmg:patch-5 branch from 8c7b9a3 to 732d09b May 10, 2019

@leofeyer leofeyer merged commit 4413eef into contao:master May 10, 2019

3 checks passed

Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
coverage/coveralls Coverage remained the same at 88.732%
Details
@leofeyer

This comment has been minimized.

Copy link
Member

commented May 10, 2019

Thank you @fritzmg.

@fritzmg fritzmg deleted the fritzmg:patch-5 branch May 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.