Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[HttpKernel] DeprecateAddAnnotatedClassesToCachePass and related code infrastructure#53801
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
ab04c1a to599867dCompare| * | ||
| * @author Fabien Potencier <fabien@symfony.com> | ||
| * | ||
| * @internal since Symfony 7.1, to be deprecated in 8.1; use Symfony\Component\DependencyInjection\Extension\Extension instead |
nicolas-grekasFeb 6, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
We cannot deprecate this class right away in 7.1 because we rely on it in FrameworkBundle and others.
Making it first internal then deprecate in 8.1 provides a smooth upgrade path.
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.
Actually, if it is already a no-op, FrameworkBundle could stop registering it.
nicolas-grekasApr 5, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
if you mean AddAnnotatedClassesToCachePass, that's done in this PR
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.
ah, I see. This is about the base class then.
Marking as@internal first which removes it from our BC policy before deprecating it only in 8.1 looks weird to me. If we don't deprecate it, we might keep it as non-internal.
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.
it's@internal since, which means its deprecated to use, but we signals to us we won't remove it in 8.0 for BC reasons
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.
but@internal since 7.1 would become@internal in 8.0, whichcan be removed.
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.
can but doesn't have to, while deprecated means it should be removed
I'm looking for a smoother plan
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.
fine by me
599867d to926be8fComparenicolas-grekas left a comment
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.
PR rebased and ready 🙏
chalasr commentedApr 6, 2024
Quick rebase needed |
…ode infrastructure
926be8f toef95928Comparefabpot commentedApr 8, 2024
Thank you@nicolas-grekas. |
…#132)`Symfony\Component\HttpKernel\DependencyInjection\Extension` isdeprecated since Symfony 7.1 bysymfony/symfony#53801
These code paths are already no-ops in 7.1 since we don't support annotations anymore.