-
-
Notifications
You must be signed in to change notification settings - Fork 449
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
Use namespaced Twig #929
Use namespaced Twig #929
Conversation
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 looks good, but the version constraint for twig in composer.json needs to be adjusted to ^1.38|^2.7
since older versions don't contain namespace classes.
Furthermore, Doctrine\Bundle\DoctrineBundle\Twig\DoctrineExtension
still makes use of the legacy class names. I would suggest changing this to namespaced classes to avoid deprecations as well. This also needs an update to composer.json, as we should not allow installing the new bundle version when an incompatible version of twig is installed. Since twig is only a dev-dependency for this package, the only way to do this without explicitly adding a requirement for twig (which is not necessary) is to create a conflict section that might be a bit more complicated to handle (we need to include twig < 1.38 and twig >= 2.0 but only < 2.7).
@alcaeus No, namespaced classes have been introduced much longer ago. |
Oh, I must have missed that then. After taking another look, aliases have been added in Twig 1.34.0 for 1.x and 2.4.0 for 2.x, so those are the versions to be added in composer.json. |
Just noticed that the twig deprecations make the tests also fail on php v7.x in the 1.10 branch. Should I change the target to 1.10.x also? |
Technically it would be possible, but I'm not sure about effectively changing the twig version requirements in a patch release, which would be necessary for this change. Instead, I'd like to change the builds to not fail on deprecations as we've done in other packages where this has become an issue in version branches. |
Well, Symfony itself already requires Twig versions shipping namespaced classes. So it is already impossible to install an older Twig version if you use a maintained version of Symfony. |
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.
You must also update the Twig extension to use namespaced classes.
Status: Needs Review |
I’ll take a look and finish this up next week, I’ll be out for a few days for personal reasons. |
No problem. Thanks for the heads up. |
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.
Changes look good, I added the appropriate conflict
section to ensure compatibility with newer twig versions and squashed commits. Thanks @rosier!
Switch to namespaced classes