Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
In #3397522: Fork Symfony's ContainerAwareTrait and ContainerAwareInterface into core we are trying to reduce the use of ContainerAwareTrait as Symfony has deprecated it.
QueueFactory is container aware because it needs to retrieve queue backend services by name. This could use a service locator instead.
However, there is only one queue backend service in core (queue.database) and there is no base class or interface to identify queue backends.
Steps to reproduce
Proposed resolution
Add an interface to queue backend services.
Add a service locator to QueueFactory.
Merge request link
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
| Comment | File | Size | Author |
|---|---|---|---|
| #19 | 3416357-nr-bot.txt | 90 bytes | needs-review-queue-bot |
Issue fork drupal-3416357
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #4
arunkumarkComment #5
SpokjeProbably wasn't ready for review yet on purpose.
Anyway, to be blatantly obvious: PHPStan baseline needs updating, CR needs to exist and linked.
Comment #7
taraskorpach CreditAttribution: taraskorpach as a volunteer and at Drupal Ukraine Community commentedBased on #5 I've removed PHPStan messages and added the mentions of QueueFactory to the main CR. So, marking it as needs review now.
Comment #8
SpokjeThanks @taraskorpach.
Hate to be that guy, but well, I _am_ that guy: New PHPStan issues showed up in the MR after removing the original one, so back to NW for that.
Comment #9
taraskorpach CreditAttribution: taraskorpach as a volunteer and at Drupal Ukraine Community commentedYes, I've seen it. Issues arose due to the missing ContainerAwareInterface and ContainerAwareTrait, along with the new paths from #3397522: Fork Symfony's ContainerAwareTrait and ContainerAwareInterface into core.
I haven't discovered what @arunkumark has done, but what would be the best solution here? Should we wait until the parent issue is merged, or should we just add the changes from this MR to the parent MR?
Comment #10
longwaveThis should use the service locator pattern, similar to #3414627: Convert StreamWrapperManager to use a service locator, rather than injecting the entire container.
Comment #11
longwaveComment #12
taraskorpach CreditAttribution: taraskorpach as a volunteer and at Drupal Ukraine Community commentedI'm not entirely confident in what I'm saying, but how can we use the service locator pattern if we don't know which services to include in the container in the register method of
ServiceLocatorTagPass?In the
getmethod, we use a service name obtained from theSettings. Does this mean we should include the entire container, or am I misunderstanding something? Please clarify this for me. Thank you in advance.Comment #13
longwaveI think we have to:
This might need to be broken into two issues, to add the interface first, and then deal with the service locator later.
Comment #14
andypostIt also needs to find contrib usage and file isssues about change
Comment #15
longwave@andypost Any ideas how to find affected contrib? There is no interface to search for, which is why we need the other issue. For example I know Redis will be affected by this, but queue_unique is not (because it just extends DatabaseQueueFactory) but I don't know of any other queue backends.
Comment #16
longwavePossibly affected contrib so far:
https://www.drupal.org/project/aws_sqs
https://www.drupal.org/project/beanstalkd
https://www.drupal.org/project/mongodb
https://www.drupal.org/project/rabbitmq
https://www.drupal.org/project/redis
Comment #17
andypostI see only 3 http://codcontrib.hank.vps-private.net/search?text=extends+QueueFactory&...
Comment #18
longwaveNow #3416826: Queue factory services do not conform to an interface has landed we can do this in 11.x, it was simpler than I first thought once I learned about
#[AutowireLocator].We could partially backport this to 10.3.x by injecting the entire container as the second constructor argument, but is it worth the effort?
Comment #19
needs-review-queue-bot CreditAttribution: needs-review-queue-bot as a volunteer commentedThe Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #20
SpokjeThere's a (IMHO) valid question from @andypost in the MR and it needs a reroll/rebase after the PHPStan-PHP-Baseline landed, but otherwise this looks fine to me.
Comment #21
longwaveRebased, responded to @andypost's question in the MR.
Comment #22
smustgrave CreditAttribution: smustgrave at Mobomo commentedI'm still amazed seeing autowire work.
Conversion seems good, the one thread has an answer as mentioned.
Comment #25
catchCommitted/pushed to 11.x and cherry-picked to 10.3.x, thanks!
Comment #27
catchRe-read things here and reverted from 10.3.x, we need to keep that constructor deprecation in and not sure it's worth the juggling to do so per #18.