Create Lazy-Loaded Twig Extension

[emmanuelballery]

Q	A
Branch?	2.x
Bug fix?	no
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#1261
License	MIT
Doc PR

Hi!

This is a new version for #1265 ;

Twig supports Lazy-Loaded Extensions since 1.35.0 and 2.4.4.

Tests have been updated too.

(Also, @mbabker provided details and Blackfire comparison in #1261)

[emmanuelballery]

Sorry @dmaicher just checked again and this question was rulled out here sulu/sulu#5556 (comment)_ in someone else attempt to stop loading this extension.

Feel free to close then, I'll disable this extension per project I guess.

[dmaicher]

@emmanuelballery I'm not a member on this repo 😉 I just stumbled across your PR as I also noticed the non-lazy twig extension and all the services that are instantiated although mostly not used in my case. So I would like to see this implemented 👍

I believe this would need to go to 3.x as its for sure a BC break.

[lsmith77]

Maybe we can figure out a non BC breaking approach and then I would be fine to have something like this in 2.x

/cc @fbourigault

[coveralls]

Coverage decreased (-0.3%) to 86.033% when pulling 3ea7028 on emmanuelballery:bugfix-twig-runtime into 018394c on liip:2.x.

[dmaicher]

Maybe we can figure out a non BC breaking approach and then I would be fine to have something like this in 2.x

/cc @fbourigault

One way of achieving this with BC would be to

• create a new Extension class like FilterRuntimeExtension or LazyFilterExtension
• deprecate the current FilterExtension and the FilterTrait
• add a config parameter liip_imagine.twig_runtime_extension: false that is false by default and deprecate not setting it to true (+ we could set it to true on the recipe)
• register the old extension if its false and the new one if its true

Then on 3.0 we could remove the old one and always use the lazy one with the runtime.

Or does someone have a better idea?

[emmanuelballery]

I'd:

• provide the new LazyFilterExtension
• deprecate the rest
• give people the choice whether they want to move on by registering the new service themselves (overriding the existing one - this way we don't add extra options to remove for 3.x)

liip_imagine.templating.filter_extension:
    class: Liip\ImagineBundle\Templating\LazyFilterExtension
    tags: [ 'twig.extension' ]

Then in 3.x register LazyFilterExtension ourselves ;

[lsmith77]

both approaches sound fine for me.

FYI if you rebase, PHPCS fixer should work again.

[dmaicher]

I would have preferred the other approach with a simpler upgrade path for the users but this is also fine for me 👍

[emmanuelballery]

@dmaicher Maybe I did not completely understand your first suggestion. I was under the impression you were asking to add a configuration and that this configuration would then be obsolete in 3.x (more work for this bundle maintainers). I chose this approach because IMO only advanced users are interested in preventing Twig from loading more container's stuff than necessary.

• The upgrade path for this bundle is only to delete what's deprecated in 3.x and uncoment the new config ;
• The upgrade path for users is to override the config now and remove (if they want) their configuration after :

    liip_imagine.templating.filter_extension:
        class: Liip\ImagineBundle\Templating\LazyFilterExtension
        tags: [ 'twig.extension' ]
    liip_imagine.templating.filter_runtime:
        class: Liip\ImagineBundle\Templating\FilterRuntime
        arguments: [ '@liip_imagine.cache.manager' ]
        tags: [ 'twig.runtime' ]

I'm completly ok to work on your approach if maintainers wants me to 👍🏻

[dmaicher]

@dmaicher Maybe I did not completely understand your first suggestion. I was under the impression you were asking to add a configuration and that this configuration would then be obsolete in 3.x (more work for this bundle maintainers). I chose this approach because IMO only advanced users are interested in preventing Twig from loading more container's stuff than necessary.

* The upgrade path for this bundle is only to delete what's deprecated in `3.x` and uncoment the new config ;

* The upgrade path for users is to override the config now and remove (if they want) their configuration after :

    liip_imagine.templating.filter_extension:
        class: Liip\ImagineBundle\Templating\LazyFilterExtension
        tags: [ 'twig.extension' ]
    liip_imagine.templating.filter_runtime:
        class: Liip\ImagineBundle\Templating\FilterRuntime
        arguments: [ '@liip_imagine.cache.manager' ]
        tags: [ 'twig.runtime' ]

I'm completly ok to work on your approach if maintainers wants me to 👍🏻

The downside of your approach is that users might see a deprecation notice since they are using the old filter. Then what do they do? They would need to find this PR (or check the CHANGELOG or so) to find out that they need to register a service themselves to fix it. Then once 3.0 is released they need to remember to remove that custom service definition again as it works without then.

So my idea was to make this a bit simpler. They get a deprecation because of the added config option. They set it to true and for 99.9% of all users this works fine. Only if someone extended the filter or so they need to rework it.
Then once 3.0 is released they get an error because of the removed config option. They remove the option and all works.
EDIT: also for new installations using the recipe we could immediately use the new filter runtime for those users (config option true)

But in the end its not for me to decide 😊 I'm fine with both options

[dmaicher]

@lsmith77 how can we move this forward? 😊 Can I help somehow?

[lsmith77]

@dbu / @Tobion

[dmaicher]

Friendly ping 😊 Can a maintainer review this?

[dbu]

sorry for the long silence. i have no some time reserved each week to keep LiipImagineBundle alive.

i added some minor comments, but like where this is going. can you please have a look at them?

and can you please rebase the branch on 2.x to get the latest changes and ci-adjustments in?

[emmanuelballery]

God ... I messed up 😞

[emmanuelballery]

Sorry for the noise ... @dmaicher is it what you were thinking of?

630016f

I am really rusty regarding bundle configuration...

[dbu]

can you please rebase on 2.x? that should improve the build situation...

i think the 8.0 test failure is because of our deprecation warnings. afaik we need to mark the tests for deprecated things with @group legacy to have phpunit not complain: https://symfony.com/doc/current/components/phpunit_bridge.html#mark-tests-as-legacy

[dbu]

great, i am happy with this now!

one last thing: can you please document the new twig_mode option in /~https://github.com/liip/LiipImagineBundle/blob/2.x/Resources/doc/configuration.rst?

[emmanuelballery]

I used this code in my projects and found some stupid mistakes ... sorry.

• enumNode in Configuration needed an array as parameters ;
• my imagine_twig_mode_lazy.xml was not referencing the new Runtime class ;
• I added documentation ;

[dbu]

awesome, thanks a lot!

[dbu]

thanks for having tested it. the mistake with the configuration would hopefully also have shown when github runs the tests. (it is always waiting for my confirmation in PRs from non repository members, afaik because some assholes started mining bitcoin on ci runners...)

can you please look at the cs fixer?

and try the @group legacy annotations?

i think the test failure is because of our deprecation warnings. afaik we need to mark the tests for deprecated things with @group legacy to have phpunit not complain: https://symfony.com/doc/current/components/phpunit_bridge.html#mark-tests-as-legacy

[dbu]

cheers! i will look into releasing a new minor version hopefully this wednesday.

[emmanuelballery]

@dbu I may have broken the configuration doc ... 😞 I guess I had to add 2 spaces before each new line for it to work.

/~https://github.com/liip/LiipImagineBundle/blob/2.x/Resources/doc/configuration.rst

[dbu]

thanks for the heads up. fixing this in #1396