[RFC] Dynamic includes in templates like {FOO}/other/stuff
- 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
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
Reason: Added [RFC] prefix to title
Re: Dynamic includes in templates of the form {FOO}/other/st
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.
- imkingdavid
- Registered User
- Posts: 1050
- Joined: Thu Jul 30, 2009 12:06 pm
Re: [RFC] Dynamic includes in templates like {FOO}/other/stu
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.
Anyway, on the topic, I agree with naderman.
- 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
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.
Re: [RFC] Dynamic includes in templates like {FOO}/other/stu
What is the connection between absolute/relative paths, or what a path is relative to, and usage of variables?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.
There may be reasons to allow variables in the path but so far I am not seeing any arguments why we should do so.
- 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
<!-- INCLUDEJS {T_ASSETS_PATH}/javascript/plupload.js --> for example.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.
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');";
}
Re: [RFC] Dynamic includes in templates like {FOO}/other/stu
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.
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.
- 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
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.
Re: [RFC] Dynamic includes in templates like {FOO}/other/stu
How do you plan to handle the assets_version part then?
- 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
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
The PR is over here: https://github.com/phpbb/phpbb3/pull/881