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

[TwigBundle] Adds bundle view path in a delayed compiler pass #30527

Open
wants to merge 16 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@alterphp
Copy link

alterphp commented Mar 11, 2019

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #30359
License MIT

Replaces #30360.

Delaies bundle view path registration into Twig filesystem loader in order to let 3rd party bundle override them if needed.

@alterphp alterphp changed the title Adds bundle view in a delayed compiler pass [TwigBundle] Adds bundle view in a delayed compiler pass Mar 11, 2019

@yceruto
Copy link
Contributor

yceruto left a comment

It looks better than before :) Thank you!

It will be good to have a test where we can see this feature in action.

@alterphp

This comment has been minimized.

Copy link
Author

alterphp commented Mar 12, 2019

It will be good to have a test where we can see this feature in action.

I'm working on it

@nicolas-grekas nicolas-grekas changed the base branch from master to 4.2 Mar 12, 2019

@nicolas-grekas nicolas-grekas added this to the 4.2 milestone Mar 12, 2019

@alterphp

This comment has been minimized.

Copy link
Author

alterphp commented Mar 12, 2019

I've added a test of the feature TwigTestExtension::testThirdPartyBundlesMayOverrideBetweenThem. It looks pretty tricky. Let me know you opinion about it.

@alterphp

This comment has been minimized.

Copy link
Author

alterphp commented Mar 12, 2019

@yceruto @nicolas-grekas Any idea on how to fix the error on AppVeyor/Travis ?

@yceruto

This comment has been minimized.

Copy link
Contributor

yceruto commented Mar 13, 2019

The .gitignore file will ignore all vendor/ sub-dirs too, try renaming the new vendor dir under Fixtures to something else.

@yceruto

This comment has been minimized.

Copy link
Contributor

yceruto commented Mar 13, 2019

@alterphp please rebase to master again and update the PR template for "master" branch and "New feature", the current one belongs to the previous PR. I guess @nicolas-grekas has been confused because of it, and has changed the base branch to 4.2.

@alterphp

This comment has been minimized.

Copy link
Author

alterphp commented Mar 13, 2019

@alterphp please rebase to master again and update the PR template for "master" branch and "New feature", the current one belongs to the previous PR. I guess @nicolas-grekas has been confused because of it, and has changed the base branch to 4.2.

That was my mistake to leave master as base branch instead of 4.2

@yceruto

This comment has been minimized.

Copy link
Contributor

yceruto commented Mar 13, 2019

It will be great to have this for 4.2, but to be honest, I think this is a new feature, there is no faulty behavior in 4.2

@stof

This comment has been minimized.

Copy link
Member

stof commented Mar 13, 2019

I think using the PrependExtensionInterface in your bundle to inject some config for TwigBundle for the path is a better way than delaying the configuration of the bundle.

@yceruto

This comment has been minimized.

Copy link
Contributor

yceruto commented Mar 13, 2019

I think using the PrependExtensionInterface in your bundle to inject some config for TwigBundle for the path is a better way than delaying the configuration of the bundle.

Yea, I thought that too (#30360 (comment)) as workaround, but when two extra bundles are involved then it is very hard to configure the paths order.

@yceruto

This comment has been minimized.

Copy link
Contributor

yceruto commented Mar 13, 2019

To be clear, this is the workaround through PrependExtensionInterface:

twig:
    paths:
        # user-configured paths
        # ...

        # added from CompanyBundle through PrependExtensionInterface
        'templates/bundles/EasyAdminBundle/': EasyAdmin # the first one wins
        'vendor/acme/company-bundle/src/Resources/views': EasyAdmin

        # added from CommonBundle through PrependExtensionInterface
        'templates/bundles/EasyAdminBundle/': EasyAdmin # it doesn't really added
        'vendor/acme/common-bundle/src/Resources/views': EasyAdmin

        # added from TwigExtension
        'templates/bundles/EasyAdminBundle/': EasyAdmin # never reached
        'vendor/easycorp/easyadmin-bundle/src/Resources/views': EasyAdmin
        'vendor/easycorp/easyadmin-bundle/src/Resources/views': !EasyAdmin

Code:

class CompanyExtension extends Extension implements PrependExtensionInterface
{
    // ...

    public function prepend(ContainerBuilder $container)
    {
        $twigConfigs = $container->getExtensionConfig('twig');

        $paths = [];
        // keeping user-configured paths
        foreach ($twigConfigs as $twigConfig) {
            if (isset($twigConfig['paths'])) {
                $paths += $twigConfig['paths'];
            }
        }
        // the overriding dir at project level (make sure the dir exists first)
        $paths['templates/bundles/TwigBundle/'] = 'EasyAdmin';
        // new overriding dir at vendor level
        $paths[\dirname(__DIR__).'/Resources/views/'] = 'EasyAdmin';

        $container->prependExtensionConfig('twig', ['paths' => $paths]);
    }
}

Same for CommonBundle.

@alterphp

This comment has been minimized.

Copy link
Author

alterphp commented Mar 13, 2019

I think using the PrependExtensionInterface in your bundle to inject some config for TwigBundle for the path is a better way than delaying the configuration of the bundle.

This is what I get by making my 2 bundles using PrependExtenionInterface :

"EasyAdmin" => array:4 [▼
      0 => "/var/www/symfony/vendor/alterphp/easyadmin-extension-bundle/src/Resources/views"
      1 => "/var/www/symfony/vendor/kiplin/admin-bundle/src/Resources/views"
      2 => "/var/www/symfony/templates/bundles/EasyAdminBundle"
      3 => "/var/www/symfony/vendor/easycorp/easyadmin-bundle/src/Resources/views"
    ]

What I want is :

"EasyAdmin" => array:4 [▼
      0 => "/var/www/symfony/templates/bundles/EasyAdminBundle"
      1 => "/var/www/symfony/vendor/kiplin/admin-bundle/src/Resources/views"
      2 => "/var/www/symfony/vendor/alterphp/easyadmin-extension-bundle/src/Resources/views"
      3 => "/var/www/symfony/vendor/easycorp/easyadmin-bundle/src/Resources/views"
    ]

2 issues :

  • the order is wrong between the 3rd-party bundles is wrong. Top-level bundle is kiplin/admin-bundle (loaded at last position), intermediate is alterphp/easyadmin-extension-bundle and at last, easycorp/easyadmin-bundle.
  • default overriding path at project level is not on top of all 3 bundles...

This could work by configuring at project-level, not trough bundle config.

@alterphp alterphp changed the base branch from 4.2 to master Mar 13, 2019

@alterphp

This comment has been minimized.

Copy link
Author

alterphp commented Mar 13, 2019

It will be great to have this for 4.2, but to be honest, I think this is a new feature, there is no faulty behavior in 4.2

Ok, done

@alterphp alterphp force-pushed the alterphp:fix/30359-2 branch from 69390e8 to e8e7c07 Mar 13, 2019

@yceruto

This comment has been minimized.

Copy link
Contributor

yceruto commented Mar 13, 2019

This could work by configuring at project-level, not trough bundle config.

@alterphp Have you tried with the code #30527 (comment) that I've prepared just for you?

the order is wrong between the 3rd-party bundles is wrong. Top-level bundle is kiplin/admin-bundle (loaded at last position), intermediate is alterphp/easyadmin-extension-bundle and at last, easycorp/easyadmin-bundle.

The order in which the extensions are executed depends on the order in which the bundles are registered. Try registering KiplinAdminBundle after EasyAdminExtensionBundle.

default overriding path at project level is not on top of all 3 bundles...

You need to add the EasyAdmin path again, see #30527 (comment).

@alterphp

This comment has been minimized.

Copy link
Author

alterphp commented Mar 13, 2019

@yceruto Yes I used what you've written especially for me ;-)

Here is EasyAdminExtensionExtension::prepend method

    public function prepend(ContainerBuilder $container)
    {
        $twigConfigs = $container->getExtensionConfig('twig');

        $paths = [];

        // keeping user-configured paths
        foreach ($twigConfigs as $twigConfig) {
            if (isset($twigConfig['paths'])) {
                $paths += $twigConfig['paths'];
            }
        }

        // Override EasyAdmin templates
        $paths[\dirname(__DIR__).'/Resources/views/'] = 'EasyAdmin';

        // Define new BaseEasyAdmin namespace to point base EasyAdmin templates
        $baseEasyAdminBundleRefl = new \ReflectionClass(EasyAdminBundle::class);
        $paths[\dirname($baseEasyAdminBundleRefl->getFileName()).'/Resources/views/'] = 'BaseEasyAdmin';

        $container->prependExtensionConfig('twig', ['paths' => $paths]);
    }

Here is KiplinAdminExtension::prepend method

    public function prepend(ContainerBuilder $container)
    {
        $twigConfigs = $container->getExtensionConfig('twig');

        $paths = [];

        // keeping user-configured paths
        foreach ($twigConfigs as $twigConfig) {
            if (isset($twigConfig['paths'])) {
                $paths += $twigConfig['paths'];
            }
        }

        // Override EasyAdmin templates
        $paths[\dirname(__DIR__).'/Resources/views/'] = 'EasyAdmin';

        $container->prependExtensionConfig('twig', ['paths' => $paths]);
    }

KiplinAdminBundle is loaded after EasyAdminExtensionBundle, let's see config/bundles.php

return [
    // ...
    EasyCorp\Bundle\EasyAdminBundle\EasyAdminBundle::class => ['all' => true],
    AlterPHP\EasyAdminExtensionBundle\EasyAdminExtensionBundle::class => ['all' => true],
    // ...
    Kiplin\AdminBundle\KiplinAdminBundle::class => ['all' => true],
    // ...
];

You need to add the EasyAdmin path again, see #30527 (comment).

This is what I dislike the most with this workaround, you have to push again a path that should, by definition, stay on top of the namespace paths stack. This is the promise of TwigBundle configuration IMO. And as a third-party bundle provider, this is a pain to explain to users that they have to add configuration to their projects to keep the default expected behavior of Twig templates cascading paths...

WDYT ?

alterphp added some commits Mar 13, 2019

@alterphp

This comment has been minimized.

Copy link
Author

alterphp commented Mar 14, 2019

As @javiereguiluz told on Slack, I ping to include this before 4.3 feature freeze ! @nicolas-grekas

@yceruto

This comment has been minimized.

Copy link
Contributor

yceruto commented Mar 14, 2019

default overriding path at project level is not on top of all 3 bundles...

You need to add the EasyAdmin path again, see #30527 (comment).

And as a third-party bundle provider, this is a pain to explain to users that they have to add configuration to their projects to keep the default expected behavior of Twig templates cascading paths

Hi @alterphp, this is what I meant #30527 (comment):

// ...

// the overriding dir at project level (make sure the dir exists first)
$paths['templates/bundles/TwigBundle/'] = 'EasyAdmin';

// ...

The user/dev doesn't care about it, they are internal details.

@alterphp

This comment has been minimized.

Copy link
Author

alterphp commented Mar 14, 2019

The user/dev doesn't care about it, they are internal details.

For project developers, why not (but it's some overhead, unoptimized memory usage anyway). As a framework, Symfony goal is also to ease the development of third-party bundles, it's not only a matter of end-users/developers.
In those cases, PrependExtensionInterface is a workaround, not a solution, IMO. And there is still the problem of the order.

@alterphp alterphp changed the title [TwigBundle] Adds bundle view in a delayed compiler pass [TwigBundle] Adds bundle view path in a delayed compiler pass Mar 14, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.