[RFC] Dynamic includes in templates like {FOO}/other/stuff

Note: We are moving the topics of this forum and it will be deleted at some point

Publish your own request for comments/change or patches for the next version of phpBB. Discuss the contributions and proposals of others. Upcoming releases are 3.2/Rhea and 3.3.
User avatar
Fyorl
Google Summer of Code Student
Posts: 27
Joined: Mon Apr 02, 2012 4:51 am
Location: UK
Contact:

[RFC] Dynamic includes in templates like {FOO}/other/stuff

Post by Fyorl »

My PR over here was originally intended to allow INCLUDEJS to include files outside of the style's template directory. I discovered the reason it was not generating file paths outside of the directory was not because of any hard-coded restriction but simply because any paths in any <!-- INCLUDE --> macro of the form '{FOO}/other/stuff' are unsupported. Is it worth extending the functionality implemented in this PR to compile_tag_include() and compile_tag_include_php()?
Last edited by imkingdavid on Sat Jul 07, 2012 1:25 am, edited 1 time in total.
Reason: Added [RFC] prefix to title

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

Re: Dynamic includes in templates of the form {FOO}/other/st

Post by naderman »

I think that would be a nice feature addition. But it should be done in a generic way that simply allows concatenation. So that "foo/{BAR}/some/{PATH}" is possible too.

User avatar
imkingdavid
Registered User
Posts: 1050
Joined: Thu Jul 30, 2009 12:06 pm

Re: [RFC] Dynamic includes in templates like {FOO}/other/stu

Post by imkingdavid »

I have added the [RFC] prefix to the topic title because it is in the RFC forum. I had to condense some wording in order for it to fix. If I changed it incorrectly, you're welcome to change it further.

Anyway, on the topic, I agree with naderman.
I do custom MODs. PM for a quote!
View My: MODs | Portfolio
Please do NOT contact for support via PM or email.
Remember, the enemy's gate is down.

User avatar
Fyorl
Google Summer of Code Student
Posts: 27
Joined: Mon Apr 02, 2012 4:51 am
Location: UK
Contact:

Re: [RFC] Dynamic includes in templates like {FOO}/other/stu

Post by Fyorl »

compile_tag_include_php doesn't appear to use get_varref at all and so will simply try to include the path passed to it verbatim. Is this desired, should there be some functionality for INCLUDEPHP to process paths like a/{FOO}/b/{BAR} ? I don't think it will hurt to have this extra functionality even if it is never used.

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

Re: [RFC] Dynamic includes in templates like {FOO}/other/stu

Post by Oleg »

Fyorl wrote: I discovered the reason it was not generating file paths outside of the directory was not because of any hard-coded restriction but simply because any paths in any <!-- INCLUDE --> macro of the form '{FOO}/other/stuff' are unsupported.
What is the connection between absolute/relative paths, or what a path is relative to, and usage of variables?

There may be reasons to allow variables in the path but so far I am not seeing any arguments why we should do so.

User avatar
Fyorl
Google Summer of Code Student
Posts: 27
Joined: Mon Apr 02, 2012 4:51 am
Location: UK
Contact:

Re: [RFC] Dynamic includes in templates like {FOO}/other/stu

Post by Fyorl »

Oleg wrote:There may be reasons to allow variables in the path but so far I am not seeing any arguments why we should do so.
<!-- INCLUDEJS {T_ASSETS_PATH}/javascript/plupload.js --> for example.

As for the connection between absolute/relative paths and usage of variables. There is no connection except that which is introduced in the code:

Code: Select all

private function compile_tag_include($tag_args)
	{
		// Process dynamic includes
		if ($tag_args[0] == '{')
		{
			$var = $this->get_varref($tag_args, $is_expr);

			// Make sure someone didn't try to include S_FIRST_ROW or similar
			if (!$is_expr)
			{
				return "if (isset($var)) { \$_template->_tpl_include($var); }";
			}
		}

		return "\$_template->_tpl_include('$tag_args');";
	}
You can see that the first character is checked for '{' and then it is assumed that the string contains only the variable and nothing else. So in this case <!-- INCLUDE {FOO} --> will work but <!-- INCLUDE {FOO}/file.html --> will not; it will in fact generate erroneous code. So currently only an absolute path, a relative path or a variable are accepted but not a mixture of a variable and absolute/relative path. My example above of INCLUDEJS shows why this is useful. It is of course possible to do the concatenation before the template is called and assign a new template variable however I don't think the mixture of variable and path is unreasonable, it seems intuitive at least to me.

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

Re: [RFC] Dynamic includes in templates like {FOO}/other/stu

Post by naderman »

Well it seems that for INCLUDEJS there should really just be 2 options: style specific js or global js in the assets directory. So that doesn't really explain why we need to support partial variables as it can easily be implemented differently.

Variables for including template files are necessary for plugin systems for example. However in those cases you can always determine the full path in code and store it in a variable, so concatenation isn't necessary.

I agree that it is bad that it creates bad php code. So it should probably at least throw an error. I still like the idea of adding support for this feature, if we can actually come up with a use case that makes sense.

User avatar
Fyorl
Google Summer of Code Student
Posts: 27
Joined: Mon Apr 02, 2012 4:51 am
Location: UK
Contact:

Re: [RFC] Dynamic includes in templates like {FOO}/other/stu

Post by Fyorl »

All JS in the assets directory is currently included with {T_ASSETS_PATH}/javascript/script.js?assets_version={T_ASSETS_VERSION} presently. It seems intuitive to me to just translate that all to <!-- INCLUDEJS {T_ASSETS_PATH}/javascript/script.js -->. I understand that all the javascript in the assets directory is in a fixed path so you could simply implement another tag or something similar but are those solutions really any more reasonable than this one? I suppose if you want to make it clear that javascript can only be included from one of two locations, an alternative implementation could do that but I think mixing variables and paths is more flexible.

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

Re: [RFC] Dynamic includes in templates like {FOO}/other/stu

Post by naderman »

How do you plan to handle the assets_version part then?

User avatar
Fyorl
Google Summer of Code Student
Posts: 27
Joined: Mon Apr 02, 2012 4:51 am
Location: UK
Contact:

Re: [RFC] Dynamic includes in templates like {FOO}/other/stu

Post by Fyorl »

The assets_version is already appended by the INCLUDEJS macro. It generates $_template->_js_include and _js_include appends ?assets_version to its argument. Since this modification I proposed still generates $_template->_js_include, this won't be a problem.

The PR is over here: https://github.com/phpbb/phpbb3/pull/881

Post Reply