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

[imkingdavid]

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()?

[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.

[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.

[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]

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.

[Fyorl]

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.

[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.

[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.

[naderman]

How do you plan to handle the assets_version part then?

[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