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

Add support for newlines with <% include %> and <% if %> #7975

Open
silbinarywolf opened this issue Apr 2, 2018 · 3 comments
Open

Add support for newlines with <% include %> and <% if %> #7975

silbinarywolf opened this issue Apr 2, 2018 · 3 comments

Comments

@silbinarywolf
Copy link
Contributor

silbinarywolf commented Apr 2, 2018

What is this?

While experimenting template parsing, I figured out to improve the flexibility of the parser to allow for newlines.

This is something that bothered me for a while so considering I've stumbled upon the knowledge to fix this, I figured I'd at least share on how we can go about making this improvement a reality.

I've got plenty on my plate in terms of "free time projects" so I'm not interested in putting in the effort for a PR / writing tests, however I've done some basic testing of grammar rule updates below and patterns that should be tested.

Understanding grammar rules quickly

"<%" = quoted string of what to consume
< = Consume optional whitespace (ie. spaces and tabs only is my understanding)
[ = Require whitespace
N = Whitespace with a newline (defined in the peg file)

Further rules are defined here.

How do I update the generated parser file?

Edit the code on this file:
https://github.com/silverstripe/silverstripe-framework/blob/4.0/src/View/SSTemplateParser.peg

Then run the following command from the framework folder to output the SSTemplateParser.php file.

composer run-script php-peg

Update <% include %>

Old Grammar Rule:

/*!*

# The include tag

Include: "<%" < "include" < Template:NamespacedWord < (NamedArgument ( < "," < NamedArgument )*)? > "%>"

*/
<% include MyInclude Arg="blah", OtherArg="hello" %>

New Grammar Rule:

/*!*

# The include tag

Include: "<%" < "include" < Template:NamespacedWord N < (NamedArgument ( < "," N < NamedArgument )*)? N > "%>"

*/

Add tests for the following:

<% include MyInclude Arg="blah", 
    OtherArg="hello" 
%>
<% include MyInclude 
    Arg="blah", 
    OtherArg="hello" 
%>

Update <% if %>

Old Grammar Rule:

/*
IfArgument: :IfArgumentPortion ( < :BooleanOperator < :IfArgumentPortion )*
// ... these two rules are in two seperate locations in code ...
IfPart: '<%' < 'if' [ :IfArgument > '%>' Template:$TemplateMatcher?
*/

New Grammar Rule:

IfArgument: :IfArgumentPortion ( N < :BooleanOperator < N :IfArgumentPortion )*
// ... these two rules are in two seperate locations in code ...
IfPart: '<%' < 'if' N < :IfArgument > '%>' Template:$TemplateMatcher?

Add tests for the following:

<% if 
$Title 
&& $Title == "About Us" %>
	Test
<% end_if %>
<% if $Title && 
$Title == "About Us" %>
	Test
<% end_if %>

Credits

@chillu
Copy link
Member

chillu commented Apr 3, 2018

Good job in digging through the PEG parser! I'm still marking this as impact/low since without a PR, I don't see a lot of value in the open source maintainers putting in the effort here (relative to other priorities)

@silbinarywolf
Copy link
Contributor Author

Fair enough!

I just wanted to capture this knowledge somewhere while it's fresh so that it's relatively easy for anyone to read this / transpose into working code.

@michalkleiner
Copy link
Contributor

I hated the single line includes for so long now, especially when we try to break down templates to as small reusable bits as possible and a lot of variables need to be passed around since includes don't fully understand the parent context.
So being able to structure includes nicely on multiple lines would be a very good improvement for us in terms of legibility of template code.

I will see if I can spare some time to create a PR for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants