[RFC|Merged] Improved template engine

These requests for comments/change have lead to an implemented feature that has been successfully merged into the 3.1/Ascraeus branch. Everything listed in this forum will be available in phpBB 3.1.
User avatar
nickvergessen
Former Team Member
Posts: 733
Joined: Sun Oct 07, 2007 11:54 am
Location: Stuttgart, Germany
Contact:

Re: [RFC|Accepted] Improved template engine

Post by nickvergessen » Mon May 16, 2011 10:20 am

nn- wrote: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
Member of the Development-TeamNo Support via PM

Oleg
Posts: 1150
Joined: Tue Feb 23, 2010 2:38 am
Contact:

Re: [RFC|Accepted] Improved template engine

Post by Oleg » Mon May 16, 2011 11:39 am

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.

User avatar
nickvergessen
Former Team Member
Posts: 733
Joined: Sun Oct 07, 2007 11:54 am
Location: Stuttgart, Germany
Contact:

Re: [RFC|Accepted] Improved template engine

Post by nickvergessen » Mon May 16, 2011 2:43 pm

Member of the Development-TeamNo Support via PM

Oleg
Posts: 1150
Joined: Tue Feb 23, 2010 2:38 am
Contact:

Re: [RFC|Accepted] Improved template engine

Post by Oleg » Fri May 20, 2011 2:49 am

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.

User avatar
AmigoJack
Registered User
Posts: 92
Joined: Wed May 04, 2011 7:47 pm
Location: グリーン ヒル ゾーン
Contact:

Re: [RFC|Accepted] Improved template engine

Post by AmigoJack » Sat Jul 23, 2011 8:00 pm

Oleg wrote: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}.

User avatar
naderman
Product Manager
Product Manager
Posts: 1727
Joined: Sun Jan 11, 2004 2:11 am
Location: Karlsruhe, Germany
Contact:

Re: [RFC|Accepted] Improved template engine

Post by naderman » Sun Jul 24, 2011 4:47 am

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
Posts: 1150
Joined: Tue Feb 23, 2010 2:38 am
Contact:

Re: [RFC|Accepted] Improved template engine

Post by Oleg » Sat Jul 30, 2011 9:29 pm

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?

User avatar
igorw
Registered User
Posts: 500
Joined: Thu Jan 04, 2007 11:47 pm

Re: [RFC|Accepted] Improved template engine

Post by igorw » Sat Jul 30, 2011 11:20 pm

Oleg wrote: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
Posts: 1150
Joined: Tue Feb 23, 2010 2:38 am
Contact:

Re: [RFC|Accepted] Improved template engine

Post by Oleg » Sun Jul 31, 2011 4:31 am

(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: :P
(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

User avatar
nickvergessen
Former Team Member
Posts: 733
Joined: Sun Oct 07, 2007 11:54 am
Location: Stuttgart, Germany
Contact:

Re: [RFC|Accepted] Improved template engine

Post by nickvergessen » Sun Jul 31, 2011 12:15 pm

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)
Member of the Development-TeamNo Support via PM

Post Reply