8000 Deprecate some interface in Twig Bridge by fabpot · Pull Request #16333 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

Deprecate some interface in Twig Bridge #16333

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

Closed
wants to merge 3 commits into from

Conversation

fabpot
Copy link
Member
@fabpot fabpot commented Oct 24, 2015
Q A
Bug fix? no
New feature? no
BC breaks? yes-ish
Deprecations? yes
Tests pass? yes
Fixed tickets n/a
License MIT
Doc PR n/a

Thanks to Twig 1.26, we can now decouple the extension from its runtime implementation. This PR does exactly that to avoid circular dependencies in the Form extension.

That's also a great way to lazy-load the runtimes.

Todo:

  • do the same for some other Twig extensions where dependencies are expensive to load.

@fabpot
Copy link
Member Author
fabpot commented Oct 24, 2015

To be rebased when #16331 is merged into 2.8. Only the second commit is relevant.

@fabpot fabpot force-pushed the form-extension-deprecations-28 branch from f982383 to e6a18b3 Compare October 24, 2015 20:35
/**
* {@inheritdoc}
*
* @deprecated Deprecated in 2.8, to be removed in 3.0.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

deprecation warnings should be added too

@stof
Copy link
Member
stof commented Oct 25, 2015

@fabpot the issue is that the DI component can deal with it only because the twig.form.engine is private and used only once, meaning it gets inlined inside the twig service. Without the inlining, things would break (and there is no way to make it work with constructor injection if twig.form.engine is not inlined, as it may be retrieved before twig).

And this is precisely why Silex won't be able to use it anymore: it does not inline services (and even setter injection does not help there, as Pimple registered the instance at the end of its instantiation, not just after the constructor like the DI component, as the whole instantiation is in the control of the user, not of Pimple)

@fabpot fabpot force-pushed the form-extension-deprecations-28 branch 3 times, most recently from 8ca71b7 to 270c970 Compare November 10, 2015 08:25
@fabpot
Copy link
Member Author
fabpot commented Nov 10, 2015

depends on twigphp/Twig#1913 to be merged first and Twig 1.24 to be released.

@fabpot fabpot force-pushed the form-extension-deprecations-28 branch from 270c970 to 0b9457d Compare November 10, 2015 08:50
*/
public function load($name)
{
$id = 'twig.extension.runtime.'.$name;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please don't force this convention: it forbids third-party bundles to use this behavior while following best practices for shared bundles. The id of the runtime should be provided as an optional attribute in the twig.extension tag IMO, to build a mapping between extension names and runtimes

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, this is just a quick POC to prove that it works.

@fabpot
Copy link
Member Author
fabpot commented Nov 10, 2015

Now, it's even better. I've removed the runtime class altogether and we reference the renderer directly, that means one less indirection and no need for the public property hack anymore.

@stof
Copy link
Member
stof commented Nov 10, 2015

@fabpot I was about to add a comment suggesting it 😄

@fabpot fabpot force-pushed the form-extension-deprecations-28 branch from 53cb4b6 to de38f6b Compare November 10, 2015 09:26
@fabpot fabpot force-pushed the form-extension-deprecations-28 branch from de38f6b to 84e0d03 Compare September 19, 2016 04:16
@fabpot fabpot changed the base branch from 2.8 to master September 19, 2016 04:16
@fabpot fabpot force-pushed the form-extension-deprecations-28 branch from 84e0d03 to 6e323f8 Compare September 19, 2016 04:39
@fabpot fabpot changed the title Deprecates some interface in Twig Bridge Deprecate some interface in Twig Bridge Sep 20, 2016
@fabpot fabpot force-pushed the form-extension-deprecations-28 branch 3 times, most recently from d204806 to c4ad497 Compare September 29, 2016 16:46
@fabpot fabpot closed this Sep 29, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants
0