-
Notifications
You must be signed in to change notification settings - Fork 194
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
fix spaceless deprecation #399
base: master
Are you sure you want to change the base?
Conversation
@@ -36,6 +36,7 @@ public function getFilters(): array | |||
{ | |||
return [ | |||
new TwigFilter('knp_menu_as_string', [$this, 'pathAsString']), | |||
new TwigFilter('knp_menu_spaceless', [$this, 'spaceless'], ['is_safe' => ['html']]), |
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.
Shouldn't we rather deprecate our compressed_root
block instead of providing our own spaceless filter suffering from the same shortcomings than the Twig one that has been deprecated because of them ?
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.
My initial idea was just about making out tests not failing about the twig deprecation, but I didn't manage to get it (using the environment variable SYMFONY_DEPRECATIONS_HELPER didn't work).
If we deprecated our internal block, we'll end up with the same problem (tests failing because of the deprecation)
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.
There's certainly room to take advantage of newer Symfony technologies in this package, although even moreso in the bundle. Like replacing the macros with Twig Components or even some twig helper functions/filters .
44a5767
to
cc14a60
Compare
Any chance this can be merged and a new release can be published? Twig 4 will be out soon, better to get ahead of this. Every one of my sites uses this wonderful library, so I see this message all the time.
|
cc14a60
to
3f35e94
Compare
I rebased this PR and updated it, now the CI passes without deprecations. The question raised by @stof is still open, though. This update would formally allow the library to be used with Twig 4, but the possible problem with the spaceless filter is not solved. |
Fix #395