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

Plugin's template cannot be overriden by theme #9654

Closed
jacquesbh opened this issue Aug 15, 2018 · 2 comments
Closed

Plugin's template cannot be overriden by theme #9654

jacquesbh opened this issue Aug 15, 2018 · 2 comments
Labels
Potential Bug Potential bugs or bugfixes, that needs to be reproduced.

Comments

@jacquesbh
Copy link
Member

Sylius version affected: 1.2.3

Description

If you want to override a plugin's template (a plugin, not a bundle), and you want to use the theme for that, it's not working.

Steps to reproduce

Let's say you install the BitBagCms plugin.

Then you want to override the template @BitBagSyliusCmsPlugin/Shop/Page/show.html.twig.
And you want to do tat in your theme.

So you create this file:

app/themes/MyTheme/BitBagSyliusCmsPlugin/Shop/Page/show.html.twig

And according to everything it should work. But it doesn't.

Possible Solution

The bug is actually in https://github.com/Sylius/Sylius/blob/1.2/src/Sylius/Bundle/ThemeBundle/Templating/TemplateNameParser.php#L65-L75.

I did that to fix the problem (very succinct):

diff --git a/src/Sylius/Bundle/ThemeBundle/Templating/TemplateNameParser.php b/src/Sylius/Bundle/ThemeBundle/Templating/TemplateNameParser.php
index 0d7fbb1f5b..22191df875 100644
--- a/src/Sylius/Bundle/ThemeBundle/Templating/TemplateNameParser.php
+++ b/src/Sylius/Bundle/ThemeBundle/Templating/TemplateNameParser.php
@@ -66,8 +66,14 @@ final class TemplateNameParser implements TemplateNameParserInterface
             return $this->decoratedParser->parse($name);
         }
 
+        if (isset($matches[1]) && preg_match('/^.+Plugin$/', $matches[1])) {
+            $bundleName = $matches[1];
+        } else {
+            $bundleName = $matches[1] ? $matches[1] . 'Bundle' : '';
+        }
+
         $template = new TemplateReference(
-            $matches[1] ? $matches[1] . 'Bundle' : '',
+            $bundleName,
             $matches[2],
             $matches[3],
             $matches[4],

It works well since we call the templates with the Plugin prefix:

@BitBagSyliusCmsPlugin/Shop/Page/show.html.twig
                ^^^^^^
@Zales0123 Zales0123 added the Potential Bug Potential bugs or bugfixes, that needs to be reproduced. label Aug 22, 2018
@chaenu
Copy link

chaenu commented Nov 24, 2018

Hello,
is this still true for version 1.3.3? I tried to overwrite templates for the CMS plugin as well, but failed. I wonder if I'm doing something wrong or if it's still this bug? Couldn't make it work with above fix either.

@tautelis
Copy link
Contributor

@chaenu sadly it is...

It used to work until 1.3.1

then after this commit only BUNDLES can be overriden in themes.
e5a5042#diff-1703885a80095c3a851cfa73b366383d

After 1.3.1 neither of those declarations can be extended in theme folder:

{% include '@MyAmazingSyliusPlugin/Page/thisNeedsToBeOverridenInTheme.html.twig' %}
{% include 'MyAmazingSyliusPlugin::Page/thisNeedsToBeOverridenInTheme.html.twig' %}

Maybe @pamil has some insight why it was done this way.

pamil added a commit that referenced this issue Jan 10, 2019
…les0123)

This PR was merged into the 1.3 branch.

Discussion
----------

| Q               | A
| --------------- | -----
| Branch?         | 1.3
| Bug fix?        | yes
| New feature?    | no
| BC breaks?      | no
| Deprecations?   | no
| Related tickets | fixes (kinda) #9654
| License         | MIT

This is, of course, a quick fix and quite straight-forward, but it works 😄 And it is still required to use `@SyliusTestPlugin` notation (maybe it's worth to be documented somehow?). This PR fixes the functionality for Sylius `^1.3`, for working fix on `1.2` take a look here: #10082.

Regarding testing it better (as I've also written in fix for `1.2`):

> It's hard to test this change with some functional test in a current tests structure, as it would require to use `SyliusPluginTrait` in test bundle/plugin, but it's in a core bundle :/ I have some idea have to test it properly with a Behat scenario, but I need to spend a few more minutes on that :)

Commits
-------

a43d08b Allow overriding templates from plugins
60b0284 Replace regexp with plain string operation
pamil added a commit to Sylius/SyliusThemeBundle that referenced this issue Jan 10, 2019
…les0123)

This PR was merged into the 1.3 branch.

Discussion
----------

| Q               | A
| --------------- | -----
| Branch?         | 1.3
| Bug fix?        | yes
| New feature?    | no
| BC breaks?      | no
| Deprecations?   | no
| Related tickets | fixes (kinda) Sylius/Sylius#9654
| License         | MIT

This is, of course, a quick fix and quite straight-forward, but it works 😄 And it is still required to use `@SyliusTestPlugin` notation (maybe it's worth to be documented somehow?). This PR fixes the functionality for Sylius `^1.3`, for working fix on `1.2` take a look here: Sylius/Sylius#10082.

Regarding testing it better (as I've also written in fix for `1.2`):

> It's hard to test this change with some functional test in a current tests structure, as it would require to use `SyliusPluginTrait` in test bundle/plugin, but it's in a core bundle :/ I have some idea have to test it properly with a Behat scenario, but I need to spend a few more minutes on that :)

Commits
-------

a43d08b24779b8cd19f678afe86e5a31853317d2 Allow overriding templates from plugins
60b0284903f4e37b58492e8df65c929652586fb8 Replace regexp with plain string operation
@pamil pamil closed this as completed in 2401264 Jan 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Potential Bug Potential bugs or bugfixes, that needs to be reproduced.
Projects
None yet
Development

No branches or pull requests

5 participants