-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
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
Conversation
To be rebased when #16331 is merged into 2.8. Only the second commit is relevant. |
f982383
to
e6a18b3
Compare
/** | ||
* {@inheritdoc} | ||
* | ||
* @deprecated Deprecated in 2.8, to be removed in 3.0. |
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.
deprecation warnings should be added too
@fabpot the issue is that the DI component can deal with it only because the 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) |
8ca71b7
to
270c970
Compare
depends on twigphp/Twig#1913 to be merged first and Twig 1.24 to be released. |
270c970
to
0b9457d
Compare
*/ | ||
public function load($name) | ||
{ | ||
$id = 'twig.extension.runtime.'.$name; |
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.
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
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.
I agree, this is just a quick POC to prove that it works.
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. |
@fabpot I was about to add a comment suggesting it 😄 |
53cb4b6
to
de38f6b
Compare
de38f6b
to
84e0d03
Compare
84e0d03
to
6e323f8
Compare
d204806
to
c4ad497
Compare
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: