silverstripe / silverstripe-framework

Silverstripe Framework, the MVC framework that powers Silverstripe CMS
https://www.silverstripe.org
BSD 3-Clause "New" or "Revised" License
721 stars 821 forks source link

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

Open silbinarywolf opened 6 years ago

silbinarywolf commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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.