[TwigBundle] do not try to register incomplete definitions #20799
Conversation
| Q | A |
|---|---|
| Branch? | 2.7 |
| Bug fix? | yes |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | #20212 |
| License | MIT |
| Doc PR |
| { | ||
| if (!interface_exists('Symfony\Component\Templating\TemplateReferenceInterface')) { | ||
| $container->removeDefinition('twig.controller.exception'); | ||
| } |
There was a problem hiding this comment.
This one looks wrong to me as the Exception controller only depends on Twig, not the Templating component.
There was a problem hiding this comment.
findTemplate() returns a TemplateReferenceInterface instance: https://github.com/symfony/symfony/blob/2.7/src/Symfony/Bundle/TwigBundle/Controller/ExceptionController.php#L99
Technically, this is not needed in 2.7 as the FrameworkBundle already requires the Templating component. But this check IMO is more correct as we do not rely on FrameworkBundle internals here.
There was a problem hiding this comment.
This should be removed in newer versions where a string is returned though.
| $container->removeDefinition('twig.controller.exception'); | ||
| } | ||
|
|
||
| if (!class_exists('Symfony\Component\Debug\Exception\FlattenException') && !interface_exists('Symfony\Component\EventDispatcher\EventSubscriberInterface')) { |
There was a problem hiding this comment.
Shoudn't it be a || instead of &&
|
When merging this PR in 3.2, we should add checks for each |
|
ping @symfony/deciders :) |
| <parameters> | ||
| <parameter key="twig.extension.form.class">Symfony\Bridge\Twig\Extension\FormExtension</parameter> | ||
| <parameter key="twig.form.engine.class">Symfony\Bridge\Twig\Form\TwigRendererEngine</parameter> | ||
| <parameter key="twig.form.renderer.class">Symfony\Bridge\Twig\Form\TwigRenderer</parameter> |
There was a problem hiding this comment.
This parameters may know not be defined while they are before, which can be a BC break. So not god for a 2.7.x release.
There was a problem hiding this comment.
I moved them back to the twig.xml file.
|
And regarding the extensions, we already have checks in a compiler pass. So I would rather fix them here if they are missing some condition rather than doing a second system in the DI extension itself. |
|
I also moved most of the clean up logic into the compiler passes except for the |
|
This one should be good to merge now. 👍 Status: Reviewed |
|
Thank you @xabbuh. |
… (xabbuh) This PR was merged into the 2.7 branch. Discussion ---------- [TwigBundle] do not try to register incomplete definitions | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #20212 | License | MIT | Doc PR | Commits ------- 2c9dc66 do not try to register incomplete definitions
…service (xabbuh) This PR was merged into the 2.8 branch. Discussion ---------- [TwigBundle] do not remove the Twig ExceptionController service | Q | A | ------------- | --- | Branch? | 2.8 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | follow up of #20799 Commits ------- c9e560f do not remove the Twig ExceptionController service
This PR was merged into the 3.1 branch. Discussion ---------- [TwigBundle][#20799] fix merge | Q | A | ------------- | --- | Branch? | 3.1 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | Commits ------- 98c835d [TwigBundle][#20799] fix merge
* 3.1: Fix merge [TwigBundle][#20799] fix merge
* 3.2: Fix merge [TwigBundle][#20799] fix merge
…(fabpot) This PR was merged into the 2.7 branch. Discussion ---------- [TwigBundle] fixed usage when Templating is not installed | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | bug introduced in #20799 | License | MIT | Doc PR | n/a In #20799, the decoupling of the Templating definition means that the `twig.loader.filesystem` is not always defined, but used in Extension. So, this PR does the opposite as what was done before. Use `twig.loader.native_filesystem` by default and copy paths to `twig.loader.filesystem` when templating is used. /cc @xabbuh Commits ------- 6aa98d1 [TwigBundle] fixed usage when Templating is not installed