Bugfix FS Loader

[peterrehm]

Due to the changes in FOSUserBundle (FriendsOfSymfony/FOSUserBundle#2378) the overriding of themes stopped working in some cases.

I digged into the code with 2 cases:

1. New way of FOSUserBundle with Template @FOSUser/Security/login.html.twig

Does not work since the call to $file = parent::findTemplate((string) $template); resolves a proper template not concerning the theme. However included templates do take the theme into account.

2. The old way with template FOSUserBundle:Security:login.html.twig

$file = parent::findTemplate((string) $template); throws an Exception which I do not understand, but the call to the parser and locater are resolving the proper template.

Reodering as done with this PR fixes that issue.

Potential side effects might be performance as the custom logic has a higher priority than the SF loader.

It is also strange that this has no effect on tests.

Tickets:

• fixes FOSUserBundle with Themes #164

[lsmith77]

@stof / @xabbuh can you chime in here .. mostly since FOSUserBundle 2.0 isn't stable I want to make sure we are not adapting to a change that might not yet have settled down.

[xabbuh]

@peterrehm I am actually not sure if I completely understood the underlying issue. Can you give a concrete example that would fail without this change?

[peterrehm]

I think the problem is for sure the implementation here in the ThemeBundle.

Assume an app with 2 themes, theme1 and theme2.

I am having a login view login.html.twig which extends a base_plain.html.twig.

With the Symfony Templating way of calling templates FOSUserBundle:Security:login.html.twig this worked like a charm. I got both templates rendered from the proper theme.

This is cause in the FilesystemLoader.php the logic as follows implemented:

• check if Twig finds a file Natively then accept that file
• if not the custom logic works well

This used to work since the Twig_Loader_Filesystem is not able to render files in the format FOSUserBundle:Security:login.html.twig and in the Exception handling the proper template will be located.

With the changes in FOSUserBundle now templates names are provided which can be resolved from the Twig_Loader_Filesystem natively and therefore the entire logic is skipped. So I am getting the default file resolved (/vagrant/src/App/UserBundle/Resources/views/Security/login.html.twig) rather than (/vagrant/src/App/UserBundle/Resources/themes/theme2/Security/login.html.twig).

But the parent file is resolved properly /vagrant/app/Resources/themes/theme2/base_plain.html.twig.

Does this make the issue more clear?

[peterrehm]

On a side note I am not sure but without further investigation I think this bundle might need some work so that the Templating Component is fully optional?

[xabbuh]

I do not know how this class initially was supposed to work so this idea might not work. But wouldn't it be better to not extend the \Twig_Loader_Filesystem class, but decorate the FilesystemLoader class from the TwigBundle instead?

[peterrehm]

I would assume it has been testes mainly against the :: notation.

Also as you can see the FilesystemLoader in this class is strongly based on the one from the TwigBundle. There it works the same, only if the Twig_Loader_Filesystem does not find a template the logic from the TwigBundle/Templating kicks in.

What do you think?

[xabbuh]

If I don't miss anything, the only thing that is different is the way cache entries are built. But in this case it IMO does not make much sense to duplicate the logic from TwigBundle, but only decorate it and populate the cache accordingly.

Though maybe @lsmith77 has some more insights in why the loader was implemented this way.

[peterrehm]

I think decorating does not make much sense from my understanding as you need to provide an entire loader class.

I think this is why the Loader of the TwigBundle works the same other than decorating the original loader.

[xabbuh]

Where is that necessary? I mean the Twig_LoaderInterface still does exist in Twig 2.

[peterrehm]

That is what I mean, in order to comply with the Twig_LoaderInterface you need to implement a full loader, where in this case it is just needed to change the logic of findTemplate().

[xabbuh]

After talking with @peterrehm about the issue I think that this PR is the right solution.

[peterrehm]

@lsmith77 What do you think?

[stof]

@lsmith77 the issue is not specific to FOSUserBundle 2. The loader of LiipThemeBundle seems to be incompatible with the native Twig namespace syntax (@FOSUser/...), which is the recommended way in the Symfony doc now (as the goal is to remove the need for the Templating component layer on top of Twig for projects using Twig).
Tests not being broken probably means that they never try using this syntax

[lsmith77]

@stof ie. you also agree that this is the right approach?

[stof]

Well, I don't think this will allow theming templates using the Twig native syntax. The parser will not parse this syntax AFAIK.
So I'm not sure this fixes the issue. It would be great to have tests proving it works (and that it continues working in the future)

[peterrehm]

This will work perfectly, I tried it in my app.

[stof]

well, a test should still be added to ensure it keeps working IMO. this is what tests are for

[peterrehm]

Oh and there is no parser in this bundle, only the locator is overriden.

[lsmith77]

@peterrehm what @stof is saying there should be a test case with the @FOSUser/Security/login.html.twig syntax

[peterrehm]

I played around with it figuring out larger issues when it comes to the test:

https://github.com/liip/LiipThemeBundle/blob/master/Tests/Locator/FileLocatorTest.php#L156

If I remove the Bundle prefix in this test, everything passes, but even when I change ThemeBundle to e.g. Thexe it passes which I do not understand. And I havent found the dime to dig further into this.

[peterrehm]

Okay, I did some further research on this matter with the conclusion this behaviour is already tested:

The current locator is designed that in case of what this package sees as bundle notation (starting with @) it will add a Bundle suffix if the suffix has been omitted.

https://github.com/liip/LiipThemeBundle/blob/master/Locator/FileLocator.php#L174

This is IMHO covered with this Test

https://github.com/liip/LiipThemeBundle/blob/master/Tests/Locator/FileLocatorTest.php#L158

where @ThemeBundle/Resources/views/template and @Theme/template have to match the same file.

If wanted I could add e.g.

        $this->assertResourcesEquals($fileLocator, '@ThemeBundle/Resources/views/template', '@Theme/Resources/views/template');

to be more explicit.

This does have a drawback as the nativ Twig syntax will be modified to the Templating syntax which works for now, but this might need a complete rewrite to Symfony's intentions in this regard: http://symfony.com/blog/new-in-symfony-2-7-twig-as-a-first-class-citizen

[lsmith77]

Hmm but why wasn't the test failing before this change?

[peterrehm]

Nope, I was wondering why it was not failing.

[peterrehm]

@lsmith77 What do you think? Any further concerns?

[lsmith77]

nope .. merged. thx

[lsmith77]

https://github.com/liip/LiipThemeBundle/releases/tag/1.4.5

[peterrehm]

👍

[xabbuh]

@peterrehm @lsmith77 I guess you will have to create a test for the loader itself to see it breaking without this change as the order in which methods are called matters for the bugfix (didn't find the time to evaluate this further though, sorry).

[peterrehm]

It should not but I just saw that I did not push the latest version of my branch to the PR.
Will open a new PR with the actual fix. Sorry for the messup.