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

Twig extensions are loaded in an uncertain order // App services are loaded after bundles ones #31841

Closed
lyrixx opened this issue Jun 4, 2019 · 13 comments

Comments

Projects
None yet
5 participants
@lyrixx
Copy link
Member

commented Jun 4, 2019

Symfony version(s) affected: 3.4+ to 4.3

Description

In older version of symfony, It was easy to override a twig function or twig filter. We had to create a new extension, register it as a service, override a filter/function, and it was done.

It worked because services were registered bundles per bundles, then the tag extraction kept the same order. And since the "AppBundle" was usually the last bundle to be registered, its services were registered lastly. And so the overriding works as expected.

How to reproduce

  1. create a new project
  2. composer req jbouzekri/phumbor-bundle
  3. composer make:twig Phumbor
    class PhumborExtension extends AbstractExtension
    {
        public function getFilters(): array
        {
            return [
                new TwigFilter('thumbor', [$this, 'thumbor']),
            ];
        }
    
        public function thumbor($value)
        {
            return 'from MY ext';
            // ...
        }
    }
  4. use the thumbor filter in a template
  5. observe that form MY ext it not displayed

Possible Solution

I did not check
Register app service after bundle ones?

Additional context

Everything is done on

$container = $this->buildContainer();
$container->compile();

The microkernel does not "lazy load" definition. Kernel::buildContainer() -> Kernel::registerContainerConfiguration() -> AppKernel::configureContainer() -> register directly the services. And during then the container compilation occurs, and bundle services's are loaded.

@lyrixx lyrixx changed the title Twig extensions are loaded in an uncertain order Twig extensions are loaded in an uncertain order // App services are loaded after bundles ones Jun 4, 2019

@lyrixx lyrixx added the HttpKernel label Jun 4, 2019

@apfelbox

This comment has been minimized.

Copy link
Contributor

commented Jun 4, 2019

A priority value would be really great for the tag, as this is useful even for third party bundles to overwrite the core functions.

@fabpot

This comment has been minimized.

Copy link
Member

commented Jun 4, 2019

Overriding functions/filters/tags is a really bad idea. It can have consequences on third-party bundles/libs and it can be a nightmare to debug.

@lyrixx

This comment has been minimized.

Copy link
Member Author

commented Jun 4, 2019

@apfelbox I thought the same thing :D We it does not exist (yet?)

@fabpot I tend to agree, but in our case, it was for adding more feature with decoration. And the previous twig filter was really simple:

https://github.com/jbouzekri/PhumborBundle/blob/2b43f5d02123189c9b84f0ba840e2f879ad543d8/Twig/PhumborExtension.php#L58-L61

More over, overloading is documented

@apfelbox

This comment has been minimized.

Copy link
Contributor

commented Jun 4, 2019

Yeah, regardless of recommendations, the documented way from Twig doesn't work reliably in Symfony. That seems to be the issue here.

@dmaicher

This comment has been minimized.

Copy link
Contributor

commented Jun 4, 2019

@fabpot

This comment has been minimized.

Copy link
Member

commented Jun 4, 2019

@lyrixx Don't forget to mention the tip "Note that overloading the built-in Twig elements is not recommended as it might be confusing.". I'm going to remove this part of the documentation. I think this is anyway too dangerous to override an existing Twig filter/tag/function (and what if more than one third-party bundle override something?). You'd better create a new one.

@fabpot

This comment has been minimized.

Copy link
Member

commented Jun 4, 2019

The more I think about this, the more I think we should forbid doing overloading and throw an exception. Do you have a real use case for overloading which cannot be solved by creating a new filter/tag/function?

@apfelbox

This comment has been minimized.

Copy link
Contributor

commented Jun 4, 2019

I can solve my use case as well via conflict in composer / renaming it, so I don't require overwriting extensions.

But I would prefer disallowing it instead of this undefined/unpredictable behavior.

@lyrixx

This comment has been minimized.

Copy link
Member Author

commented Jun 4, 2019

Do you have a real use case for overloading which cannot be solved by creating a new filter/tag/function?

We are using thumbor to resize our picture "on the fly". This is quite powerful. So almost everywhere in our application we have suce code:

img src="{{ thumbor(media.path) }}" alt="{{ media.alt }}" />

We added a new feature in ou backoffice: user can now upload SVG and use them anywhere. But thumbor does not support SVG (this make sens).

So i have overloaded the twig function to handle both cases:

    public function transform($origin, $transformation = null, $overrides = [])
    {
        if (is_string($origin) && '.svg' === substr($origin, -4)) {
            return $this->router->generate('public_media_svg', [
                'path' => substr($origin, 0, -4),
            ]);
        }
        return $this->transformer->transform($origin, $transformation, $overrides);
    }
@fabpot

This comment has been minimized.

Copy link
Member

commented Jun 4, 2019

So, you can keep your code but name the function differently. That looks saner to me.

@lyrixx

This comment has been minimized.

Copy link
Member Author

commented Jun 4, 2019

Wep, Basically, this is all about laziness :)

@fabpot

This comment has been minimized.

Copy link
Member

commented Jun 5, 2019

see https://github.com/twigphp/Twig/pull/3053/files for the deprecations of Twig overloading.

@fabpot fabpot closed this Jun 5, 2019

@stof

This comment has been minimized.

Copy link
Member

commented Jun 5, 2019

@fabpot regarding overloading, DebugBundle overloads the dump function (registered initially by Twig's DebugExtension) to make it use VarDumper instead of using var_dump. What would be the solution there (we cannot avoid registering Twig's DebugExtension as Twig Environment is doing it automatically)

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.