-
-
Notifications
You must be signed in to change notification settings - Fork 5.3k
[Asset][Lock][RateLimiter] Use the #[Target]
attribute in some examples
#21382
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
Conversation
#[Target]
attribute in some examples
#[Target]
attribute in some examples#[Target]
attribute in some examples
#[Target]
attribute in some examples#[Target]
attribute in some examples
#[Target]
attribute in some examples#[Target]
attribute in some examples
I completed the pending work. This is now ready for review. Thanks. |
lock.rst
Outdated
class SomeService | ||
{ | ||
public function __construct( | ||
#[Target('invoice.lock.factory')] private LockFactory $invoiceLockFactory |
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.
#[Target('invoice.lock.factory')] private LockFactory $invoiceLockFactory | |
#[Target('invoice.lock.factory')] private LockFactory $lockFactory |
Like this to avoid confusion with the argument name used in the example before?
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.
#[Target('invoice.lock.factory')] private LockFactory $invoiceLockFactory | |
#[Target('invoice')] private LockFactory $lockFactory |
And reading the code of the FrameworkExtension
class I wonder if this would work too.
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.
Yes, but that only works in 7.4 and up because of #21134.
Here we're documenting the old way of doing it so we can update this in 7.4 to show the better way. Thanks.
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.
Oh indeed, but I guess renaming the argument would still be a good idea.
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.
Absolutely! Good catch.
I just updated the argument names of the Target
examples. Thanks for reviewing it!
class SomeService | ||
{ | ||
public function __construct( | ||
#[Target('foo_package.package')] private PackageInterface $fooPackage |
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.
#[Target('foo_package.package')] private PackageInterface $fooPackage | |
#[Target('foo_package.package')] private PackageInterface $package |
to avoid confusion with the previous argument
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.
reading the code of the FrameworkExtension
class this should be even easier like this:
#[Target('foo_package.package')] private PackageInterface $fooPackage | |
#[Target('foo_package')] private PackageInterface $package |
ae9daad
to
5d24590
Compare
In #21134 we need to document how much easier is in 7.4 to pick the implementation using the
#[Target]
attribute. But, we don't have many examples of using that attribute in the first place. So, let's add it now and we'll later improve this in 7.4 branch and up.Still WIP because we need to show the same for Lock and Semaphore.