[dev] Horde_Template::

[Michael M Slusarz]

Rather than talk about Horde_Template:: issues on several different  
threads (and several different mailing lists), I'm going to try to  
respond in a single message.

1. The new changes don't break BC.  As mentioned on the ingo@ list, BC  
was already broken by some changes made a year or so ago.  Thus, any  
Horde 3.x application that needs to use if/loops within a loop needs a  
copy of Horde_Template:: locally to work properly.  The recent changes  
in Horde_Template did not break any templates, but simply required us  
to maintain an entire copy of Horde_Template:: in each application  
since some of the private class methods have changed.

In Ingo, for example, we were using Ingo_Template:: for templates with  
loops.  Ingo_Template:: replaced the loop processing function with a  
function that contained the previously-mentioned BC-breaking changes.   
However, this function was calling functions in Horde_Template:: that  
don't exist anymore - but only if you were using the latest CVS  
checkout.  Therefore the need to copy the entire copy of  
Horde_Template to each application.

2. Chuck is correct - I may have been overly greedy with removing the  
'if' parameter from set() in IMP.  We can only remove the 'if'  
parameter if we are using the newest version of the Template library.   
This can be easily fixed by simply pointing all templating code to  
IMP_Template:: rather than Ingo_Template::.

3. Attached below are some snippets from another mailing list about  
some issues with the old/new/any templating system to be used in Horde:

==========

> I'd like to share my opinion with you about your first reason:
> "protecting designers from PHP code".
>
> I remember having discussions with you 3 years ago defending this  
> reason. By that time I transformed some of the files to Horde  
> Templates (login, mailbox, view message, etc), hoping to have an easy
>  life afterwards by thinking that the designers would be able to  
> change the templates. Oh god, I was so wrong, it didn't help at  
> all... A lot of work for nothing... but I learned a lession...

Well, good to know I guess. :)

> Since then I changed my opinion about it. What I think it's important
>  now is to separate the presentation from everything else. Basically,
>  to have a distinct View component, like in the  MVC design-pattern  
> model. And this model is perfect for me when you are able to install  
> the core of your application in a central location, and then have  
> several different interface designs in different locations using the  
> same core.

Yup, that's the ideal/goal.

> Another big problem I had 3 years ago was HTML code in the middle of =20
> the libs.. That was a big head ache because I had two completely =20
> different layouts for the Webmail.

Yup.

> As for a template engine, and because of everything I just said, I  
> don't really thing it's needed at all. It just slows down the code. I
>  only defend a Templating engine for a different purpose - to  
> standardize templates within an organization. In my case, all the PT  
> code uses smarty (and I think even perl code uses smarty templates as
>  well). The engine is powerful enough for you to do whatever you want
>  to do in the View component without the need of tweaking a lot of  
> code in the core, just because the engine is not flexible enough.

I know a lot of people like Smarty, but it's just about the complete
opposite of where I want to go with templates. PHP is a perfectly good
language; I have no desire to learn another kinda-similar but not as
powerful one, when we can just write our templates in PHP.

Still need to seperate them out and be diligent about it, but there
_is_ such a thing as display logic, and Smarty has php code plugins,
so... six of one, I guess.

> Ok, I just contributed my 2 cents about this subject.. :-)

I'm well over, I'm guessing. :)

==========

>> As far as the suggestion that there should be a sub-class that  
>> "compiles" the file, I disagree.  Maintaining 2 different ways to  
>> parse a template file (i.e. the old way, "compiling") doesn't make
>> any  sense.  Additionally, the old way of parsing had the problems  
>> identified above, not to mention was not very  
>> performance-efficient.  Additionally, maybe "compilation" is a bad  
>> term to be using - all
>> the  new code is doing is parsing, albeit in a way which we can
>> potentially  cache as opposed to before (where we were compiling and
>> running  simultaneously).
>
> It's turning the template into PHP code - for the purposes of  
> template engines, that's compiling.
>
> Also, I have to say that it's a _significant_ disadvantage that this  
> now uses eval(). There were two reasons to use Horde_Template  
> previously: protecting designers from PHP code (what designers? nice  
> idea, but we're the ones writing all of these templates), and  
> allowing safe user-submitted templates. Now that the templates are  
> compiled into PHP and then eval'd, you still get the non-PHP syntax  
> for templates, but that's not a huge advantage. I'd frankly rather  
> just use PHP templates for anything that's performance critical.
>
> I'm serious; I'd rather have a simple View wrapper like  
> http://solarphp.com/trac/browser/trunk/Solar/View.php (I'm sure I  
> don't agree with all of Paul's design decisions, but that's the  
> general idea) than a limited template syntax that goes through eval().

I'll take a look at this article later.  But I appreciate concerns =20
with eval(), and will need to look at this some more.

>> As far as I can see, these are three test cases that don't work  
>> with the current code, but I don't see a tremendous issue:
>>
>> #1: $foo =3D array('a' =3D> 1, 'b' =3D> 1);
>> <tag:foo.a />
>> <tag:foo.b />
>>
>> However, this is *explicitly* not allowed according to the wiki  
>> documentation.  So I am not tremendously concerned about this right  
>> now (we don't use this format anywhere in IMP.)
>
> This was specifically added at someone's request, I believe. It's  
> pretty useful.

The new code now handles this so this isn't an issue anymore.

>> Additionally, other test cases in Template/tests don't work due to an
>> issue with the way PHP parses a file.  For example, the following
>> code:
>> <tag:foo1 />CRLF
>> <tag:foo2 />
>>
>> Will render as this:
>> foo1foo2
>>
>> That is because this PHP does not treat the EOL in this code as whitespac=
e:
>> <?php echo 'foo1'; ?>CRLF
>> <?php echo 'foo2'; ?>
>>
>> Thus, none of the tests will work properly.  Either of two
>> workarounds  1.) live with this behavior - if you equate a template
>> file with  nothing more than "shorthand for PHP code" than this is
>> fine. 2.)
>
> Templates should be able to output plain text, though.

Based on your other comments on dev@, this will need to be looked at
also.  But right now, we only use Horde_Template:: to output HTML
code, so we really just need something

To me it really comes down to a simple fact of life - the old code was
terribly slow (as mentioned ad nauseum previously, it was taking up to
1/3 of runtime to generate the finished product).  Reducing a runtime
by a factor of 60 can't be ignored.  Additionally, the limitations of
the other template system - if you make a single typo in a template
file, you can crash the server with infinite loops - don't make it
100% stable either.

I would be all for adopting some other kind of templating system -
realizing the tradeoffs of additional installation requirements and
possible overhead - but at the advantage of a more stable and/or
secure codebase.  But at this stage of the release cycle, we shouldn't
be re-rewriting all of our templates so an intermediate solution,
while maybe not optimal, seems like the best choice.

We can always back out the commit (and the corresponding if-statement
cleanups) if needed.

==========

[And further comment re: the last message]

>> Templates should be able to output plain text, though.
>
> Based on your other comments on dev@, this will need to be looked at
> also.  But right now, we only use Horde_Template:: to output HTML
> code, so we really just need something

I may go back on myself on this one; if you had a regular php file with:
<?php echo 'Foo' ?>
<?php echo 'Bar' ?>

.. you'd need to put in \n yourself to preserve the newline. So while
it's not ideal for plain text templates, if we're not using them
currently, oh well.

> To me it really comes down to a simple fact of life - the old code
> was terribly slow (as mentioned ad nauseum previously, it was taking
> up to 1/3 of runtime to generate the finished product).  Reducing a
> runtime by a factor of 60 can't be ignored.  Additionally, the
> limitations of the other template system - if you make a single typo
> in a template file, you can crash the server with infinite loops -
> don't make it 100% stable either.

Yeah, and I should have said more/at all that that's an incredible
improvement, and awesome work by you and thanks to Nuno/PT for the
seed of the code for it.

> I would be all for adopting some other kind of templating system -
> realizing the tradeoffs of additional installation requirements and
> possible overhead - but at the advantage of a more stable and/or
> secure codebase.  But at this stage of the release cycle, we
> shouldn't be re-rewriting all of our templates so an intermediate
> solution, while maybe not optimal, seems like the best choice.

Yes. I think I'd like to advocate for not introducing _more_ use of
Horde_Template at this point in the release cycle, also. But of course
you're right. I'd like to have a Horde_View or similar for Horde 4.0,
with support for a nice bundle of templates, with css, images, and
perhaps javascript included, that can be duplicated, re-used, etc.
(and is, by default anyway, all PHP code for the template language -
no overhead, no installation requirements).

==========

If Chuck wants to talk more about his conceptual ideas for Horde_View,  
that would be great. :)  This thread might also be ripe for the wiki,  
or at least be linked to on any Horde 4.0 discussion page.

michael

___________________________________
Michael Slusarz [slusarz at horde.org]