SSViewer Patch

205 views
Skip to first unread message

Hamish Campbell

unread,
Nov 26, 2009, 2:32:46 PM11/26/09
to SilverStripe Development
Hi all,

I've just submitted a fairly significant patch for SSViewer:

http://open.silverstripe.org/ticket/4766

The actual changes are fairly minor, but there are lots of them:

- Removes deprecated ereg and ereg_replace calls (from 5.3.0)
- Optimizes other small bits and pieces
- Added some new functionality mentioned further below.

This dramatically improves template compilation (blackcandy compiles
in about a tenth of the time, from some basic tests) and I've added
comments for each replacement so you can see what all the currently
available template commands are. I've derived them from the regex and
I think they're correct, but they need to be reviewed.

The change also opens up the possibility of extending/modifying the
templating behavour. Some more work would be needed to create a proper
API, but it does allow you to, for example, append new templating
rules if you so wish.

The included features are:

+ A new <% parent %> tag. See http://open.silverstripe.org/ticket/4751

This allows you to traverse up the template item stack to access
parent View items. Currently this isn't possible from the template
system - E.g. while looping through a many-many child object, you
can't refer to the current parent item.

+ Make Requirements calls conditional. See http://open.silverstripe.org/ticket/4188

This was a change submitted by Simon W that fits into the patch
nicely. This slightly changes the way Requirements are added to the
template, by evaluating them within the other control structures
rather than etracting them all and executing them. This means that a
Requirements block within a <% if %> block will only be evaluated if
the <% if %> is executed.

+ Tests

Tests for the vast majority of template tags don't exist. I started on
some basic tests as part of 4751 (and Simon included tests for the
changes mentioned din 4188), but we really a complete test suite for
such a basic component. Anyone want to help?

So, what does everyone think?

smurkas

unread,
Nov 26, 2009, 2:58:34 PM11/26/09
to SilverStripe Development
This sounds really awesome!

The new <% parent %> tag is going to be very useful, I've felt the
need for it several times and had to resort to other solutions to get
around it.
I'm checking out the code now from the repository so I'm hoping to
give this a whirl tonight or during the weekend.

Is there any chance that 3738 will be making it into 2.4?
This is one of the most important patches for me that would be kind of
game changing for what you can write in the template.

With the awesome work you've done here and patch 3738 you'd start to
give Smarty a run for its money ;)

On Nov 26, 8:32 pm, Hamish Campbell <hn.campb...@gmail.com> wrote:
> Hi all,
>
> I've just submitted a fairly significant patch for SSViewer:
>
> http://open.silverstripe.org/ticket/4766
>
> The actual changes are fairly minor, but there are lots of them:
>
> - Removes deprecated ereg and ereg_replace calls (from 5.3.0)
> - Optimizes other small bits and pieces
> - Added some new functionality mentioned further below.
>
> This dramatically improves template compilation (blackcandy compiles
> in about a tenth of the time, from some basic tests) and I've added
> comments for each replacement so you can see what all the currently
> available template commands are. I've derived them from the regex and
> I think they're correct, but they need to be reviewed.
>
> The change also opens up the possibility of extending/modifying the
> templating behavour. Some more work would be needed to create a proper
> API, but it does allow you to, for example, append new templating
> rules if you so wish.
>
> The included features are:
>
> + A new <% parent %> tag. Seehttp://open.silverstripe.org/ticket/4751
>
> This allows you to traverse up the template item stack to access
> parent View items. Currently this isn't possible from the template
> system - E.g. while looping through a many-many child object, you
> can't refer to the current parent item.
>
> + Make Requirements calls conditional. Seehttp://open.silverstripe.org/ticket/4188

Hamish Campbell

unread,
Nov 26, 2009, 3:49:33 PM11/26/09
to SilverStripe Development
Hadn't seen that - it's excellent work and definitely where SSViewer
should be heading.

Many one of the core devs can let us know where that one is at?

Robbie MacKay

unread,
Nov 26, 2009, 3:57:17 PM11/26/09
to silverst...@googlegroups.com
This all sounds like great stuff!
Good job guys :)

Robbie MacKay
> --
>
> You received this message because you are subscribed to the Google Groups "SilverStripe Development" group.
> To post to this group, send email to silverst...@googlegroups.com.
> To unsubscribe from this group, send email to silverstripe-d...@googlegroups.com.
> For more options, visit this group at http://groups.google.com/group/silverstripe-dev?hl=en.
>
>
>

Sam Minnee

unread,
Nov 26, 2009, 5:31:33 PM11/26/09
to SilverStripe Development
> I've derived them from the regex and
> I think they're correct, but they need to be reviewed.

Tests are the only way that we can reliably ensure that these are
correct.

> The change also opens up the possibility of extending/modifying the
> templating behavour. Some more work would be needed to create a proper
> API, but it does allow you to, for example, append new templating
> rules if you so wish.
>
> The included features are:
>
> + A new <% parent %> tag. Seehttp://open.silverstripe.org/ticket/4751
>
> This allows you to traverse up the template item stack to access
> parent View items. Currently this isn't possible from the template
> system - E.g. while looping through a many-many child object, you
> can't refer to the current parent item.

I agree that this is a very useful feature, but I'm not sure about the
naming. We currently have $Top - I was thinking that $Up might be
better? This would distinguish it from <% control Parent %> more
explicitly too.

Additionally, I don't think it should be a separate <% keyword %>. <
% control Up %> or $Up.Title would be more in keeping with the rest of
the template language.

> + Make Requirements calls conditional. See http://open.silverstripe.org/ticket/4188
> This was a change submitted by Simon W that fits into the patch
> nicely. This slightly changes the way Requirements are added to the
> template, by evaluating them within the other control structures
> rather than etracting them all and executing them. This means that a
> Requirements block within a <% if %> block will only be evaluated if
> the

Good to have them included. :-)

> Tests for the vast majority of template tags don't exist. I started on
> some basic tests as part of 4751 (and Simon included tests for the
> changes mentioned din 4188), but we really a complete test suite for
> such a basic component. Anyone want to help?

Yeah, the lack of tests is a major reason why the SSViewer system has
stagnated - without tests, it's very dangerous to muck with such a
byzantine piece of code. One word of caution: ensure that we test the
output of the template engine, and not the generated PHP. Otherwise
we won't be able to use the tests for more radical re-workings of the
template engine. In addition, tests on the output will be easier to
write.

We're going to need more a few more tests before we could merge this
patch. Hamish, the comments on your patch give us a nice list of the
things that we need to test. Although there are a big list of items,
it shouldn't be too hard to extend SSViewerTest to include test cases
for these.

Ideally, we should build the test as a separate patch, apply them to
the current template engine and confirm that they work, and then apply
your template engine upgrades.

<%-- Template Comment --%>
<% debug %>
<% debug Foo %>
<% base_tag %>
<% current_page %>
$Iteration
{$Foo(Arg1, Arg2).Bar.Val}
{$Foo(Arg1, Arg2)}.Val}
{$Foo(Arg1, Arg2)}
{$Foo(Arg1).Bar.Val}
{$Foo(Arg1).Bar}
{$Foo(Arg1)}
{$Foo.Bar.Val}
{$Foo.Bar}
{$Foo}
$Foo.Bar(Arg1)
$Foo.Bar(Arg1, Arg2)
$Foo(Arg1, Arg2).Bar.Val
$Foo(Arg1, $Arg2).Bar
$Foo(Arg1, Arg2)
$Foo(Arg1).Bar.Val
$Foo(Arg1).Bar
$Foo(Arg1)
$Foo.Bar.Val
$Foo.Bar
$Foo
<% control Foo %>
<% control Foo.Bar %>
<% control Foo.Bar(Arg1) %>
<% contorl Foo(Arg1) %>
<% control Foo(Arg1, Arg2) %>
<% control Foo(Arg1, Arg2, Arg3) %>
<% end_control %>
<% parent %>
<% end_parent %>
<% if Foo % >
<% else_if Foo %>
< % if Foo.property %>
<% else_if Foo.property %>
<% if Foo.Bar.Val %>
<% else_if Foo.Bar.Val %>
< % if Foo(parameter) %>
< % if Val1 || Val2 %>
<% else_if Val1 || Val2 %>
< % if val1 && val2 %>
<% else_if Val1 && Val2 %>
< % if Val1 == Val2 %>
<% else_if Val1 == Val2 %>
< % if Val1 != Val2 %>
<% else_if Val1 != Val2 %>
<% else %>
<% end_if %>
<% _t(...) %> with entity only (no dots in namespace). current
template filename will be added as a namespace
<% _t(...) %>
<% sprintf(_t(...),$argument) %> with entity only (no dots in
namespace). current template filename will be added as a namespace
<% sprintf(_t(...),$argument) %>

Sam Minnee

unread,
Nov 26, 2009, 6:10:24 PM11/26/09
to SilverStripe Development
> Ideally, we should build the test as a separate patch, apply them to
> the current template engine and confirm that they work, and then apply
> your template engine upgrades.

Okay, I'm making some really good progress in this arena; I'll let you
know how I get on.

Hamish Campbell

unread,
Nov 26, 2009, 6:47:31 PM11/26/09
to SilverStripe Development
Thanks Sam,

> > + A new <% parent %> tag. Seehttp://open.silverstripe.org/ticket/4751
> >
> > [snip]
>
> Additionally, I don't think it should be a separate <% keyword %>.   <
> % control Up %> or $Up.Title would be more in keeping with the rest of
> the template language.

I was about to agree then realized that it might not be possible.
Traversing up the stack is (effectively) the opposite operation to <%
control %>. <% end_control %> pops an item off the stack, whereas <%
end_parent %> pushes the item to the stack.

We can catch <% control Up %> as a special case, but I can't see how
you could reliably handle differences in <% end_control %>.

$Up.Property makes sense though, and shouldn't be a problem.

Sam Minnee

unread,
Nov 26, 2009, 6:57:56 PM11/26/09
to SilverStripe Development
> I was about to agree then realized that it might not be possible.
> Traversing up the stack is (effectively) the opposite operation to <%
> control %>. <% end_control %> pops an item off the stack, whereas <%
> end_parent %> pushes the item to the stack.
>
> We can catch <% control Up %> as a special case, but I can't see how
> you could reliably handle differences in <% end_control %>.

We will probably need to push the previous context onto an internal
stack at the beginning of each control, and pop it from the internal
stack at the end. Up can then make use of this stack to return the
parent context.

To sketch it out (this won't work as-is, because currentTemplate()
doesn't exist and itemStack isn't a member variable).

ViewableData::Up() {
return SSViewer::currentTemplate()->getContextAbove($this);
}

SSViewer::getContextAbove($item) {
foreach($this->itemStack as $i => $stackItem) {
if($stackItem === $item) return ($i>0) ? $this->itemStack[$i-1] :
null;
}
}

* By iterating through absolute stack each item, we can ensure that
$Up.Up.Title gets the grandparent title
* By comparing $stackItem === $item, we don't need to inject any meta-
data into the ViewableData class itself.

Michael Gall

unread,
Nov 26, 2009, 7:09:02 PM11/26/09
to silverstripe-dev
There's an existing bug that I did a bit of work on ages ago.

http://open.silverstripe.org/ticket/1934

Your solution here is much simpler than what I did in that, originally I was looking at not having to specify parents and searching up through the stack at each variable call. That would have broken backwards compatibility, but I think would make for a much nicer looking templates.



--

You received this message because you are subscribed to the Google Groups "SilverStripe Development" group.
To post to this group, send email to silverst...@googlegroups.com.
To unsubscribe from this group, send email to silverstripe-d...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/silverstripe-dev?hl=en.





--
Checkout my new website: http://myachinghead.net
http://wakeless.net

Sam Minnee

unread,
Nov 26, 2009, 7:19:29 PM11/26/09
to SilverStripe Development
> Your solution here is much simpler than what I did in that, originally I was
> looking at not having to specify parents and searching up through the stack
> at each variable call. That would have broken backwards compatibility, but I
> think would make for a much nicer looking templates.

The problem with magically looking for matching values would be
ambiguity:

<% control Parent %>
$Up.Title - $Title
<% end_control %>

Not to mention that the template system doesn't really clearly
distinguish between "That variable isn't defined" and "That variable
is blank". Which would lead to some weird bugs.

I think Up is a simple way of dealing with this.

Sigurd Magnusson

unread,
Nov 26, 2009, 7:22:19 PM11/26/09
to silverst...@googlegroups.com
Agreed! Thanks very much Hamish - your contribution is very much
appreciated.
Eliminating the use of features marked as deprecated in PHP 5.3 is a
nice bonus, thanks!

Sigurd.

Sam Minnee

unread,
Nov 26, 2009, 7:27:08 PM11/26/09
to SilverStripe Development
Okay, I've
http://open.silverstripe.org/changeset/93718
http://open.silverstripe.org/changeset/93719
http://open.silverstripe.org/changeset/93720
http://open.silverstripe.org/changeset/93721

I've managed to cover most of the template engine pretty quickly. The
main thing still outstanding is the testing of <% _t %>.
The approach I've taken to making the tests is to build a flexible
SSViewerTestFixture object that can easily supply the various tests
with appropriate data. That is set up in r 93718.

From the previous list, this is what is covered:

<%-- Template Comment --%>
Requirements
<% base_tag %>

Nivanka Fonseka

unread,
Nov 26, 2009, 8:27:03 PM11/26/09
to silverst...@googlegroups.com
Sam,

does it means SilverStripe templates can now take multiple arguments?
previously the comma didnt work. is it possible now?

Cheers

Nivanka

--

You received this message because you are subscribed to the Google Groups "SilverStripe Development" group.
To post to this group, send email to silverst...@googlegroups.com.
To unsubscribe from this group, send email to silverstripe-d...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/silverstripe-dev?hl=en.





--
Nivanka Fonseka
Senior Software Engineer

Skype: nivanka.fonseka
Twitter: @nivankafonseka

Sam Minnee

unread,
Nov 26, 2009, 8:31:36 PM11/26/09
to SilverStripe Development

> does it means SilverStripe templates can now take multiple arguments?
> previously the comma didnt work. is it possible now?

I haven't made any changes to the template engine itself, only added
the tests. Read over those tests to see what's possible.

Hamish Campbell

unread,
Nov 26, 2009, 8:53:26 PM11/26/09
to SilverStripe Development
On Nov 27, 1:27 pm, Sam Minnee <sam.min...@gmail.com> wrote:
> I've managed to cover most of the template engine pretty quickly.  The
> main thing still outstanding is the testing of <% _t %>.
> The approach I've taken to making the tests is to build a flexible
> SSViewerTestFixture object that can easily supply the various tests
> with appropriate data.  That is set up in r 93718.

Thanks Sam - it's really great to see this move so quickly.

On Nov 27, 1:27 pm, Sam Minnee <sam.min...@gmail.com> wrote:
> We will probably need to push the previous context onto an internal
> stack at the beginning of each control, and pop it from the internal
> stack at the end. Up can then make use of this stack to return the
> parent context.

This is what I was doing with the $childStack in 4751

> SSViewer::getContextAbove($item) {
> foreach($this->itemStack as $i => $stackItem) {
> if($stackItem === $item) return ($i>0) ? $this->itemStack[$i-1] :
> null;
> }
> * By iterating through absolute stack each item, we can ensure that
> $Up.Up.Title gets the grandparent title
> * By comparing $stackItem === $item, we don't need to inject any meta-
> data into the ViewableData class itself.

After working through this, I think it might be a bit fragile. It
assumes that $item will only appear in the stack once, or if it
appears more than once, will always have the same parent [as the first
occurance]. This is a false assumption - there are situations where an
object could be pushed to the array multiple times and not necessarily
with the same template parent. This will cause problems finding the
correct context.

Of course, if you iterate backwards instead, you can no longer do Up
more than once, since you'll keep returning the same objects (you've
created a loop).

My solution in 4751 was a result of trying a few different methods and
I believe it is solid. By storing the childStack you can return to it
on <% end_parent %> without any fuss. Basically, you traverse up and
down without adding any extra objects. But it highlights the
fundamental difference between <% control %> and what <% parent %> was
trying to achieve.

Sam Minnee

unread,
Nov 26, 2009, 9:31:01 PM11/26/09
to SilverStripe Development
> > I've managed to cover most of the template engine pretty quickly.  The
> > main thing still outstanding is the testing of <% _t %>.
> > The approach I've taken to making the tests is to build a flexible
> > SSViewerTestFixture object that can easily supply the various tests
> > with appropriate data.  That is set up in r 93718.
>
> Thanks Sam - it's really great to see this move so quickly.

Yeah, test coverage of SSViewer was long overdue. ;-)

> After working through this, I think it might be a bit fragile. It
> assumes that $item will only appear in the stack once, or if it
> appears more than once, will always have the same parent [as the first
> occurance]. This is a false assumption - there are situations where an
> object could be pushed to the array multiple times and not necessarily
> with the same template parent. This will cause problems finding the
> correct context.

So, the plot thickens! I'm still not happy with the <% parent %> tag,
though. One of the major problems with the <% parent %> .. <%
end_parent %> syntax is that it's poorly suited to the situation where
you just want to grab one variable from the parent context. Can you
imagine doing something like this with <% parent %> ...<% end_parent
%>

<div id="{$Up.ID}_$ID" class="$Up.CSSClass">

I think it we can get some test cases for these edge-cases spelled
put, we'll be in a better position to solve this with a function Up()
{ ... } approach. We may have to bit the bullet and add some state
data to ViewableData, though. I'll give it some thought.

Hamish Campbell

unread,
Nov 29, 2009, 5:06:28 PM11/29/09
to SilverStripe Development
Morning everyone,

Have updated http://open.silverstripe.org/ticket/4766 with minor fixes
required to pass the new SSViewer tests.

Wow, those tests highlight lots of issues with the current rules
though :D.. even just fixing those up will be a good win.

Sam Minnee

unread,
Nov 29, 2009, 5:21:29 PM11/29/09
to SilverStripe Development
> Wow, those tests highlight lots of issues with the current rules
> though :D.. even just fixing those up will be a good win.

Would you be able to produce a patch that excludes the <% parent %>
stuff? I feel like we're still working through that part, but the
remainder of the fixes can be applied sooner than that.

Hamish Campbell

unread,
Nov 29, 2009, 5:28:37 PM11/29/09
to SilverStripe Development
On Nov 27, 3:31 pm, Sam Minnee <sam.min...@gmail.com> wrote:
> So, the plot thickens!  I'm still not happy with the <% parent %> tag,
> though.  One of the major problems with the <% parent %> .. <%
> end_parent %> syntax is that it's poorly suited to the situation where
> you just want to grab one variable from the parent context.  Can you
> imagine doing something like this with <% parent %> ...<% end_parent
> %>
>
> <div id="{$Up.ID}_$ID" class="$Up.CSSClass">

$Up.blah is definitely the easier case - there is always the option
that we handle $Up.blah but not <% control Up %>. Maybe <% control Up
%> isn't worth it for the extra complexity?

> We may have to bit the bullet and add some state
> data to ViewableData, though.

Don't you still have same item in the stack at different places..
presumably with the same state data?

Sam Minnee

unread,
Nov 29, 2009, 5:43:28 PM11/29/09
to SilverStripe Development
I was thinking about this over the weekend; one strategy would be to
create an separate object like this:

function Up() {
return SSViewer_UpHandler();
}

class SSViewer_UpHandler {
function __construct($item, $levelsUp = 1) {
$this->item = $item;
$this->levelsUp = $levelsUp;
}
function Up() {
return new SSViewer_UpHandler($this->levelsUp + 1);
}
function obj(), cachedCall(), XML_Val(); // pass through to
appropriate item in context stack.
}

That way you're not relying on tying behaviour to the SSViewer
objects.

----

However, this raises another issue. I think that it would be a good
move to replace the base class ViewableData with a separate wrapper
object that SSViewer instantiated around each item processed by the
template, that implemented cachedCall, XML_val, obj(). Alternatively,
those methods are implemented on the SSViewer class itself and have
the template content passed to them.

This would make it much easier to pull data from other sources into
the template (no more ArrayData, just give an array/object), as well
as giving us the opportunity to shrink the inheritance hierarchy.

This being the case, it might be better to deal with Up() after
refactoring things in that way.

Michael Gall

unread,
Nov 29, 2009, 5:50:37 PM11/29/09
to silverstripe-dev
However, this raises another issue.  I think that it would be a good
move to replace the base class ViewableData with a separate wrapper
object that SSViewer instantiated around each item processed by the
template, that implemented cachedCall, XML_val, obj().  Alternatively,
those methods are implemented on the SSViewer class itself and have
the template content passed to them.

I think this would be a good move. Would really help to separate the template and model logic.

 
This would make it much easier to pull data from other sources into
the template (no more ArrayData, just give an array/object), as well
as giving us the opportunity to shrink the inheritance hierarchy.

This being the case, it might be better to deal with Up() after
refactoring things in that way.
--

You received this message because you are subscribed to the Google Groups "SilverStripe Development" group.
To post to this group, send email to silverst...@googlegroups.com.
To unsubscribe from this group, send email to silverstripe-d...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/silverstripe-dev?hl=en.





--

Sam Minnee

unread,
Nov 29, 2009, 5:54:53 PM11/29/09
to SilverStripe Development
> However, this raises another issue.  I think that it would be a good
> move to replace the base class ViewableData with a separate wrapper
> object that SSViewer instantiated around each item processed by the
> template, that implemented cachedCall, XML_val, obj().  Alternatively,
> those methods are implemented on the SSViewer class itself and have
> the template content passed to them.

Note that before we make such a change, we would want to ensure that
we have template engine tests for:

* Custom exists methods
* Casting from string to object (ViewableData::$casting and
DataObject::$db)
* Casting form object to string (ViewableData::forTemplate())
* Escaping of non-HTML fields.
* Overloading of obj(), XML_val(), cachedCall()

Sam Minnee

unread,
Nov 29, 2009, 6:05:37 PM11/29/09
to SilverStripe Development
> I think this would be a good move. Would really help to separate the template and model logic.

Exactly. It would enable the use of our ORM without the template
engine, and the template engine without the ORM. At a high-level, it
moves us in the direction of a more of a loosely coupled architecture.

I suspect that method-calls on an SSViewer instance will be more
efficient that instantiating a wrapper object for every element in a
template.
Reply all
Reply to author
Forward
0 new messages