[RFC|Merged] Improved template engine

[nickvergessen]

One of the last remaining issues is the $orig_tpl_inherits_id variable. It seems to me that it should go away, but I'm having a hard time figuring out a use case when it will be actually used. I will add a test for template inheritance and kill $orig_tpl_inherits_id off most likely.

Bug: http://tracker.phpbb.com/browse/PHPBB3-8688
Fix: http://code.phpbb.com/projects/phpbb/re ... ions/10150

[Oleg]

The diff tells me that the issue originally was with writing to $user from template, which is something that should not be done in the first place.

And the origin of $orig_tpl_inherits_id is here:

http://tracker.phpbb.com/browse/PHPBB3-8092
http://code.phpbb.com/projects/phpbb/re ... /9839/diff

It is really not easy figuring out why all of these things are necessary without appropriate comments. My overall direction of thought is to delete all writes to $user from template and therefore $orig_tpl_inherits_id with it. Tests should be added for all use cases, which should be rather straightforward. The only use case I have at this point however is just straightforward inheritance.

[nickvergessen]

Also see http://www.phpbb.com/bugs/phpbb3/ticket ... t_id=40515 for some more comments.

[Oleg]

I reopened the pull request: https://github.com/phpbb/phpbb3/pull/171

I added tests for inheritance and absolute path php file inclusion.

I attempted pulling loader/locator out of template class but it turned out to be a pretty big mess. For now let's see if we can live without them.

The template engine is blocking hooks and db storage work, I'd like to get the patch-so-far merged and possibly (I hope for probably) revisit it after hooks. DB storage work probably would not be impacted by further changes to the template engine and in fact it may require some changes to the template engine.

[AmigoJack]

the template engine expects $this to have template variables, which in the executor/renderer is not the case. Therefore I will be adding a "context" class which will store variables used during template evaluation.

So this issue with the current 3.0.x template system will be resolved then?

Edit: just reviewed the GIT pull for /includes/template/filter.php and still see the old behaviour on line 449. My main interest is to use properties instead of variables everywhere in the rendered PHP code, so scope limitations won't affect file inclusions anymore. Thus included template files can also reference loop (template) variables, like {postrow.POST_ID}.

[naderman]

That does seem like a useful improvement. Changing the semantics for 3.0 is probably not a good idea, but we should get this into 3.1. Rather than writing to tpldata we should probably come up with a better way to set values, including DEFINE'd values that does not require public access to the template context.

[Oleg]

I factored out the template locator into its own class.

Can someone clarify the purpose of orig_tpl_inherits_id, introduced in https://github.com/ur/phpbb3/commit/89332c00 ? Currently that functionality is broken, yet no tests fail. If it is important (i.e. applies to non-db stored templates) we need to add tests for it.

I'm considering making phpbb_template an interface so that tpl_include/php_include functions can continue to be public on the template (implementation) class but not part of the public interface, what does everyone think about this idea?

[igorw]

I'm considering making phpbb_template an interface so that tpl_include/php_include functions can continue to be public on the template (implementation) class but not part of the public interface, what does everyone think about this idea?

I don't think it's worth adding an interface just for this. But there's no harm done either, so I'm not strongly against it.

[Oleg]

(20:52:13) naderman: nn-: https://github.com/ur/phpbb3/commit/6e3 ... 1bf6e0a36b
(20:52:57) naderman: although that does not specifically point out what orig_tpl_inherits_id is
(20:53:16) naderman: basically the template engine can be used with a different template than what was loaded
(20:53:35) naderman: and in that case all the template specific info (like storedb, inherits_id, ...) is stored in orig_* variables
(20:54:00) naderman: or not
(20:54:05) naderman: actually I think I am wrong about that too
(20:54:11) naderman: I can't remember what exactly it does
(20:54:12) naderman:
(20:55:51) naderman: nn-: oh now I remember
(20:55:59) naderman: the template engine has some hard coded references to $user
(20:56:10) naderman: but for some stuff you want to ignore the theme_ values in $user
(20:56:14) naderman: so they get overwritten
(20:56:31) naderman: and the orig_ stuff is there to restore those values in $user
(20:56:41) naderman: it's a mess
(20:56:49) naderman: and if done properly those references to $user would just be removed
(20:59:08) nx-: ok, that is a little too much for me at the moment
(20:59:14) nx-: (the commit message)
(20:59:41) naderman: heh yeah
(20:59:47) naderman: it took me forever to figure that out
(20:59:47) naderman:
(20:59:49) nx-: so basically we both concur that it is a mess
(21:00:16) naderman: it is
(21:00:26) nx-: i want to see new template instances created for independent template invocations
(21:00:35) nx-: unless for some reason this is not possible
(21:00:36) naderman: anyway at least now I know it was a good decision to explain that properly when i commited it
(21:00:45) naderman: although adding tests & and some comments wouldn't have hurt
(21:00:55) naderman: nx-: yes absolutely
(21:01:01) naderman: I don't see why not
(21:01:26) naderman: " To make this work the template class now always
(21:01:26) naderman: overwrites $user->theme storedb and inheritance variabbles with orig_tpl_* just
(21:01:26) naderman: before rendering a template in _tpl_load. This way they are guaranteed to always
(21:01:26) naderman: contain the value they had at the time set_template/set_custom_template were
(21:01:26) naderman: called. This fixes [Bug #54505]."
(21:01:31) nx-: In r10150 these orig_tpl_* variables are correctly used to access information about the template of the page being displayed - contrary to the last template used - from within the bbcode, -- wtf
(21:02:03) naderman: lol
(21:02:09) naderman: hmm
(21:02:18) naderman: I guess I was properly confused when I was writing that
(21:02:35) naderman: I think what I meant is:
(21:02:51) igorw: bbcode also needs to fall back to the parents' bbcode.html
(21:03:15) naderman: in r10150 the orig_tpl_* variables are used to access information about the template of the page being displayed. It does not use the values manually overwritten for bbcode, which would be the last template used at that time.
(21:03:57) naderman: yeah I did make quite a few typos etc. there
(21:03:58) naderman: :/
(21:04:12) nx-: i'm not really able to comprehend this at the moment
(21:04:17) naderman: lol
(21:04:29) naderman: yeah, kind of need to look at all those commits to get it I guess
(21:04:30) nx-: naderman, if you feel like at least describing a test case it would be awesome

[nickvergessen]

The variables were added for the bbcode.html and custom templates.
There was an error in the ACP, when the style was inheriting (f.e. prosilver SE) and you visited the ACP > User > Signature where the BBCodes were displayed.
We needed the original styles data in that case so we could display the BBCodes from the original template. (Proper fix could have been to add a bbcode.html to the adm/style aswell)