Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
Twig extensions refatoring to decouple definitions from implementations#20093
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
5253e33 to27fd008Compare| { | ||
| if (null !==$this->renderer) { | ||
| @trigger_error(sprintf('Passing a Twig Form Renderer to the "%s" constructor is deprecated since version 3.2 and won\'t be possible in 4.0. Pass the Twig_Environment to the TwigRendererEngine constructor instead.',get_class($this)),E_USER_DEPRECATED); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
could usestatic::class instead ofget_class($this).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
done
| * | ||
| * Unfortunately Twig does not support an efficient way to execute the | ||
| * "is_selected" closure passed to the template by ChoiceType. It is faster | ||
| * to implement the logic here (around 65ms for a specific form). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It is faster to implement the logic as function (around 65ms for a specific form).
as function explains a lot more at once, took me a bit to actually grasp the relation between this function andgetTests without
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This part is not new, just moved in the file. I would say that we could removed it entirely, WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It does give a nice explanation ofwhy it's chosen to be a function instead of closure/class object property. If you're referring to the 3 paragraphs, I don't think they add a real value to the docblock, but a smallThis is a function and not callable due to performance reasons. would be good to prevent people from making PRs to refactor it again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
done
linaori commentedSep 29, 2016
I have the feeling#20092 serves as a base for this PR? I noticed duplicate changes |
27fd008 to31fe9e2Comparefabpot commentedSep 29, 2016
Yes, I will rebase this one when#20092 is merged (probably in the next hour or so if I don't have any other comments). |
ea5a669 to7973629Comparefabpot commentedSep 29, 2016
Rebased now. |
768427d to0b76c8dCompare| * | ||
| * @see ChoiceView::isSelected() | ||
| */ | ||
| publicfunctionisSelectedChoice(ChoiceView$choice,$selectedValue) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
to be more BC (in case someone calls it directly), we could make it a static method rather than a function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is clearly not an extension point, and as performance for this one is critical (the reason why we have this in the first place), I prefer to keep the function.
| */ | ||
| private$template; | ||
| publicfunction__construct(array$defaultThemes =array(),\Twig_Environment$environment =null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Should we trigger a deprecation when the environment is not injected in the constructor ? It would make sense to remove the setter in 4.0 IMO. The environment is a required dependency, and replacing it after we started using the renderer may break things due to the caching of the block resolution
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
done
| $compiler | ||
| ->addDebugInfo($this) | ||
| ->write('$this->env->getExtension(\'Symfony\Bridge\Twig\Extension\FormExtension\')->renderer->setTheme(') | ||
| ->write('$this->env->getRuntime(\'Symfony\Bridge\Twig\Form\TwigRenderer\')->setTheme(') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this requires bumping the min requirement for Twig
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What do you mean? That's already the case (1.26)
182edfa to22e45c0Compare22e45c0 to2d01b80Compare2d01b80 tob515702Compare…m implementations (fabpot)This PR was merged into the 3.2-dev branch.Discussion----------Twig extensions refatoring to decouple definitions from implementations| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | seetwigphp/Twig#1913| License | MIT| Doc PR | not yetThis PR tries to use the new Twig runtime loader to the Twig bridge extensions.Commits-------b515702 fixed circular reference in Twig Form integration4b5e412 removed on indirectionc07fc58 [TwigBridge] decoupled Form extension definitions from its runtime
… (chalasr)This PR was merged into the 1.x branch.Discussion----------Add a simple Twig_RuntimeLoaderInterface implementationNext tosymfony/symfony#21023This is related to the BC break reported insymfony/symfony#21008 which has been introduced insymfony/symfony#20093 when decoupling extensions from definitions.What I propose here is to ease the upgrade to symfony 3.2+ by adding a simple `Twig_RuntimeLoaderInterface` implementation here, useful only when using the twig-bridge outside of the symfony fullstack framework (with the Form component for instance).Upgrading would be as simple as:```diff$twig = new Twig_Environment(...);$rendererEngine = new TwigRendererEngine(array('form_div_layout.html.twig'), $twig);- $twig->addExtension(new FormExtension(new TwigRenderer($rendererEngine, $csrfTokenManager)));+ $twig->addExtension(new FormExtension());+ $twig->addRuntimeLoader(new Twig_RuntimeLoader(array(TwigRenderer::class => new TwigRenderer($rendererEngine, $csrfTokenManager)));```Instead of having to write this runtime loader yourself.Please seesymfony/symfony#21008 for details and a concrete example of how this could help.Commits-------91c8d59 Add a Twig_FactoryRuntimeLoader
…tEnvironment() (chalasr)This PR was merged into the 3.3-dev branch.Discussion----------[TwigBridge] Late deprecation for TwigRendererEngine::setEnvironment()| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | yes| Tests pass? | yes| Fixed tickets |#20093 (comment)| License | MIT| Doc PR | n/aThis method should have been deprecated in 3.2 since the twig environment should be injected through the constructor and replacing it later can break things (see#20093 (comment) for details).Maybe this could target 3.2Commits-------aabb73c Deprecate TwigRendererEngine::setEnvironment()
This PR tries to use the new Twig runtime loader to the Twig bridge extensions.