Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Regression in 3.1.28 - extendsall resource type before template file name doesn't work #123

Closed
chilek opened this issue Dec 14, 2015 · 34 comments

Comments

@chilek
Copy link

chilek commented Dec 14, 2015

When we use

{include file="extendsall:template.html"}

in our template, extendsall is not supported at all in 3.1.28.
File which defines extendsall resource type is original resource.extendsall.php file from tgz package (demos/plugins/resource_extendsall.php) and is placed in /path/to/application/plugins directory added to plugin dirs by the following code:

$smarty->addPluginsDir('/path/to/application/plugins');

In 3.1.27 workaround with prefilter replacing {include file="template.html"} by {include file="extendsall:template.html"} was a solution.

@uwetews
Copy link
Contributor

uwetews commented Dec 15, 2015

I can't reproduce your problem. If I use something like

{include file="extendsall:template.html"}

the resource gets loaded from the plugins folder.

Note: The file name is resource.extendsall.php, not resource_extendsall.php or was it just a typo?

I also do not understand your comment regarding the prefilter under 3.1.27.
Or do you modify Smarty::$default_resource_type dynamically?

@chilek
Copy link
Author

chilek commented Dec 15, 2015

First of all - of course I've made mistake with extendsall resource plugin file - it was and it's named resource.extendsall.php ;-)

In 3.1.27 I've used the following code:

class Smarty_Prefilter_Extendsall_Include {
    static public function prefilter_extendsall_include($tpl_source, Smarty_Internal_Template $template) {
        if (is_array($template->smarty->template_dir) === false)
            return $tpl_source;
        // prepend all files in {include} blocks with resource type 'extendsall:'
        return preg_replace('#(\{include\s*file=[\'"])(?:(?![a-z]+:|/|\{))(.+)([\'"][^}]*\})#i', '$1extendsall:$2$3', $tpl_source);
    }
}

to automatically prepend template file name with 'extendsall:' in all inclusions inside template file.
That doesn't work anymore with 3.1.28 :(
Simply I don't want to change all include's in templates prepending their names with 'extendsall'.
How could I do that with 3.1.28?

@uwetews
Copy link
Contributor

uwetews commented Dec 15, 2015

How did you make the filter known?
I tried

 $smarty->registerFilter('pre',array('Smarty_Prefilter_Extendsall_Include', 'prefilter_extendsall_include'));

and it worked.

@chilek
Copy link
Author

chilek commented Dec 16, 2015

Exactly the same as you wrote.
You mean 'it worked' as prepended all included templates with 'extendsall:' and loaded them properly?

@chilek
Copy link
Author

chilek commented Dec 16, 2015

Uwe, have a look at problem demonstration which I've prepared. You can download it from:
https://www.chilan.com/smarty-3.1.28-extendsall-test.tar.xz

@uwetews
Copy link
Contributor

uwetews commented Dec 17, 2015

You have general design problem.
By using extendsall in your example "data1.html" get loaded from both template folders template1 and template2. "data1.html" contains {extends 'data1.html'}.
Two problems:
A) A template should never call extends with same name.
B) Using extendsall on templates which contain {extends} with same template name will create endless loops.
It did work accidentally in previous Smarty versions by the way how Smarty resolved {block} tag replacement at compile time.
With Smarty 3.1.28 template inheritance was changed to run time processing the resolved previous shortcomings. Now all the design flaws are popping up.

I would also get rid of the pre-filter as you may get results you don't expect by the template code.

@chilek
Copy link
Author

chilek commented Dec 17, 2015

The assumption with previous smarty version was that when I extend 'data1.html' first time I can modify content from dynamically loaded plugins - templates2 in such example is plugin template directory (first directory from template directory list), so data1.html from templates2 had been used already and smarty, knowing about that, took into account data1.html from templates1 avoiding endless loop / recursion. That was very cool feature which allows us to construct plugins which hide original template from application core, but at the same time allow to use template content from core without rewriting all...
Uwe, how could I do that with Smarty 3.1.28?

@uwetews
Copy link
Contributor

uwetews commented Dec 17, 2015

I need to think about it, might take a few days....

@chilek
Copy link
Author

chilek commented Dec 17, 2015

Nice to hear that. Uwe, who personally gets payments after clicking "Donate" (Paypal) on smarty.net www site?

@uwetews
Copy link
Contributor

uwetews commented Dec 17, 2015

Perhaps I get a Christmas present.... lol
I think I do understand your basic idea. But as I said before your solution did work so far by some internal behavior of Smarty and not really because of template and application code. Lets see if ther is a clean solution.

@uwetews
Copy link
Contributor

uwetews commented Dec 18, 2015

Why do you need the {extends 'data1.html'} in templates2/data1.html ?
If you remove that it does work.
Because of some patches use the master branch version for testing.

@chilek
Copy link
Author

chilek commented Dec 18, 2015

I've asked about "Donate" button, because I'd like to pay voluntarily 10$ monthly. I know that open source projects always suffer from underfunding, so when most smarty users would pay 10$ monthly (I'm pretty sure they do their bussiness quite well thank to Smarty) the project would be much more vital ;-)

@chilek
Copy link
Author

chilek commented Dec 18, 2015

Why do you need the {extends 'data1.html'} in templates2/data1.html ?
If you remove that it does work.
Because of some patches use the master branch version for testing.

So if I'll remove {extends file="data1.html'} from templates2/data1.html from templates1/data1.html
it will take templates2/data1.html and append its content to templates1/data1.html?
So I'd use more template dirs i.e. templatesN/data1.html, ..., templates3/data1.html, templates2/data1.html, template1/data1.html smarty will use all data1.html in "chain" as they appear?

@uwetews
Copy link
Contributor

uwetews commented Dec 19, 2015

As I said before {extends} must never use same template name as the calling one.
Because of your template_dir order {extends file="data1.html'} from templates2/data1.html it does find
templates2/data1.html as the first matching file and so calling itself and causing an endless loop.
The extendsall resource does load all template files in chain as they appear. It works similar to the extends resource where all temple names are the same but uses the template_dir order to define child and parent relation ships.
In your case the {extends} is only useful if you need to extend a template file which will be not loaded by extendsall.

Instead of using the prefilter can

$smarty->setDefaultResourceType('extendsall');

to change the default resource type from file to extendsall.

Your donates will be always wellcome.

@chilek
Copy link
Author

chilek commented Dec 19, 2015

Why do you need the {extends 'data1.html'} in templates2/data1.html ?
If you remove that it does work.
Because of some patches use the master branch version for testing.

As I see when I'll use data1.html localised by template dir chain in this order:

templates2/data1.html, templates1/data1.html 

templates2/data1.html will be loaded only and templates1/data1.html will not?
When I used:

{extends file="data1.html"}

both file were loaded, and first one could manipulate block contents defined in second one.
When I remove above {extends ...} from templates2/data1.html it won't be possible to modify templates1/data1.html and in fact this last file will not be loaded at all.
Am I missed something?

In Smarty < 3.1.28:

$smarty->setDefaultResourceType('extendsall');

caused template file load errors so as workaround I had to use prefilter.
I'll try this again with 3.1.28 and with master branch head.

@chilek
Copy link
Author

chilek commented Dec 19, 2015

Ok I've changed index.php from previous test application to:

include('smarty.git/libs/Autoloader.php');
Smarty_Autoloader::register();

$SMARTY = new Smarty;

$SMARTY->addPluginsDir('SmartyPlugins');

$SMARTY->setDefaultResourceType('extendsall');

$SMARTY->setTemplateDir(null);
$SMARTY->AddTemplateDir(array('templates2', 'templates1'));
$SMARTY->setCompileDir('templates_c');

$SMARTY->display('template.html');

and this is the only changed file.
I tried smarty 3.1.28 and smarty from git master HEAD (smarty.git is clone of github's smarty).
In both cases I got HTTP 500 error.
Then commented out:

$SMARTY->setDefaultResourceType('extendsall');

and with git master HEAD the generated content is blank (no HTTP error). With 3.1.28 content is generated partly - only from header block :(

@uwetews
Copy link
Contributor

uwetews commented Dec 20, 2015

$SMARTY->setDefaultResourceType('extendsall');

sorry, it did require a bugfix which is now in the master branch.

You must use the newest resource.extendsall.php which you did not in the provided example

@chilek
Copy link
Author

chilek commented Dec 20, 2015

I did:

git pull

on smarty repo local copy and it seems to work ;-) Thanks a lot, Uwe!

@uwetews uwetews closed this as completed Dec 20, 2015
@uwetews
Copy link
Contributor

uwetews commented Dec 20, 2015

Anyway it was an interesting use case of extendsall.
So I did learn also a bit. :)

@chilek
Copy link
Author

chilek commented Dec 20, 2015

I've found another bug: when we prepend template dir chain with templates_3 directory and put data1.html file there with the following content:

{block name="content-data1" prepend}
<p>data1 from templates3</p>
{/block}

after reload page in web browser we get HTTP 500 error. To resolve the problem we have to clear smarty compiled templates' cache. Then it works again. Any change in template dir chain requires clearing cache :(

@chilek
Copy link
Author

chilek commented Dec 20, 2015

PHP error log shows:

[20-Dec-2015 15:18:19 Europe/Warsaw] PHP Fatal error:  Uncaught  --> Smarty: Unable to load template file '[1]template.html' in 'template.html' <-- 
  thrown in /var/www/chilan.com/htdocs/smarty-test/Smarty/libs/sysplugins/smarty_internal_template.php on line 139

It seems that templates are not recompiled "magically" after template directory chain change, because [1]template.html is invalid if we have 3 template dirs in chain.

@uwetews
Copy link
Contributor

uwetews commented Dec 20, 2015

Grr I see what happens when you change your template_dir settings. I have today no time left. I will look into it tomorrow

@chilek
Copy link
Author

chilek commented Dec 20, 2015

No problem - there is no pressure :) Thanks in advance for problem investigation.

uwetews added a commit that referenced this issue Dec 20, 2015
@uwetews
Copy link
Contributor

uwetews commented Dec 20, 2015

The fix (resource.extendsall.php) is now in the master branch

@chilek
Copy link
Author

chilek commented Dec 20, 2015

I confirm that commit mentioned above resolves my problem.

@uwetews
Copy link
Contributor

uwetews commented Dec 21, 2015

Me again.
There is another update in the master branch with speed optimizations.

You may also use

$smarty->setMergeCompiledIncludes(true);

With this setting Smarty create a single compiled file containing all template code.
This executes much faster as just one file has to me loaded.

Another note: You may place a {strip} tag at the beginning of the template files to get rid of empty lines, or enable it for certain section with {strip} {/strip}.

btw I added your special use case to our PHPunit tests. Hopefully we not run again in future into problems like this

Regars Uwe

@chilek
Copy link
Author

chilek commented Dec 21, 2015

Great to hear that - after git pull works like a charm ;-)
Thanks a lot again.

@uwetews
Copy link
Contributor

uwetews commented Dec 21, 2015

By the way, I have released 3.1.29

@chilek
Copy link
Author

chilek commented Dec 21, 2015

Wow, I'll update smarty requirement in my application very soon ;-)

@uwetews
Copy link
Contributor

uwetews commented Dec 21, 2015

It took a while to sort all things out. But the case to use extendsall over all levels of templates and sub-templates including inheritance was also a bit new to me. I think we have now removed all ambiguousness in template and script code,

Small hint

$SMARTY->setTemplateDir(null);
$SMARTY->addTemplateDir(array('templates2', 'templates1'));

can be replaced by

$SMARTY->setTemplateDir(array('templates2', 'templates1'));

@chilek
Copy link
Author

chilek commented Dec 21, 2015

Uwe, in this example we can do that, but in my real application plugins add their own template directories and I accidentally left this as two method calls ;-)

@chilek
Copy link
Author

chilek commented Jan 18, 2016

Hi Uwe, I've tried to port my application to Smarty 3.1.29 with hope that everything will be all right, but it didn't play well :(
I've prepared demo to demonstrate issue:
https://www.chilan.com/smarty-test2/
All files can be downloaded from:
https://www.chilan.com/smarty-3.1.29-extendsall-test.tar.xz
This time not all templates are loaded. I've found that commenting out the following line from templates/default/layout.html file:

{include file="toolbar.html"}

causes loading all other contents after include.
Is it a bug in Smarty or bug in my template?

@chilek
Copy link
Author

chilek commented Jan 29, 2016

Hello, hello? Anybody's here? ;-)

@chilek
Copy link
Author

chilek commented Feb 5, 2016

Hello, Uwe.... ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants