-
-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
[HttpKernel] Deprecate AddAnnotatedClassesToCachePass
and related code infrastructure
#53801
Conversation
ab04c1a
to
599867d
Compare
@@ -17,6 +17,8 @@ | |||
* Allow adding classes to the class cache. | |||
* | |||
* @author Fabien Potencier <fabien@symfony.com> | |||
* | |||
* @internal since Symfony 7.1, to be deprecated in 8.1; use Symfony\Component\DependencyInjection\Extension\Extension instead |
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.
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, which can 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
to
926be8f
Compare
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 🙏
Quick rebase needed |
…ode infrastructure
926be8f
to
ef95928
Compare
Thank you @nicolas-grekas. |
…#132) `Symfony\Component\HttpKernel\DependencyInjection\Extension` is deprecated since Symfony 7.1 by symfony/symfony#53801
These code paths are already no-ops in 7.1 since we don't support annotations anymore.