-
Notifications
You must be signed in to change notification settings - Fork 191
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
.github/workflows/build.yaml
Outdated
@@ -4,6 +4,9 @@ on: | |||
pull_request: ~ | |||
push: ~ | |||
|
|||
env: | |||
SYMFONY_DEPRECATIONS_HELPER: max[self]=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.
I would not add this globally. The existing behavior is that other jobs report all deprecations as being an issue, and that's the goal. We want the CI to report direct deprecations (to avoid our users complaining that KnpMenu triggers deprecations when using it because our CI would not have warned us).
The -prefer-lowest
job uses a special config restricting only our own deprecations and not direct or indirect ones, because we cannot expect the lowest dependency version to be deprecation-free.
.github/workflows/build.yaml
Outdated
dev: true | ||
symfony: 7.0.* | ||
symfony: 7.1.* |
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.
the job using dev deps should not force a Symfony version at all (especially given that this is not the dev version of Symfony)
@@ -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)
Fix #395