Problem/Motivation

In #3049525: Enable service autowiring by adding interface aliases to core service definitions we enabled core.services.yml for autowiring by adding aliases to several core interfaces and classes. We can extend this by doing the same for core modules.

Steps to reproduce

Proposed resolution

Extend AutowireTest to cover core modules as well as core.services.yml.
Add aliases to module interfaces and classes where appropriate.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#8 3325557-nr-bot.txt150 bytesneeds-review-queue-bot

Issue fork drupal-3325557

Command icon 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:

About issue forks
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

longwave created an issue. See original summary.

longwave opened merge request !3075

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Active » Needs review

Rajeshreeputra made their first commit to this issue’s fork.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs review » Needs work

longwave credited kim.pepper.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs work » Needs review

Crediting @kim.pepper for review, thanks for spotting that; back to NR.

Removing credit for @Rajeshreeputra for an unnecessary reroll.

needs-review-queue-bot’s picture

needs-review-queue-bot CreditAttribution: needs-review-queue-bot as a volunteer commented
Status: Needs review » Needs work
FileSize
3325557-nr-bot.txt150 bytes

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs work » Needs review
mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented
Status: Needs review » Reviewed & tested by the community

Looks good to me. RTBC.

Going OT a bit: how about https://symfony.com/doc/current/service_container/autowiring.html#dealin...? This would help autowiring for cache bins and for loggers like e.g.

  cache.default:
    class: Drupal\Core\Cache\CacheBackendInterface
    tags:
      - { name: cache.bin }
    factory: ['@cache_factory', 'get']
    arguments: [default]
  Drupal\Core\Cache\CacheBackendInterface $cacheDefault: '@cache.default'
longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented

Thanks. Not sure yet what approach would work for cache bins (feel free to open a followup!), but for loggers I think binding a default per module could work, see #3295542-3: Use logger channel services consistently, instead of the factory object.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented
Status: Reviewed & tested by the community » Needs work

Needs rebasing

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs work » Reviewed & tested by the community

Rebased, back to RTBC.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Amber Himes Matz’s picture

Amber Himes Matz
she/her
Primary language English
Location Portland, OR USA
CreditAttribution: Amber Himes Matz at Drupalize.Me commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll, +Needs Review Queue Initiative

1. Looks like there is 1 unresolved comment (that got addressed but was never marked as resolved): https://git.drupalcode.org/project/drupal/-/merge_requests/3075#note_137045)

2. The merge request is currently blocked and needs a rebase.

Thank you!

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs reroll

Rebased and resolved the comment, back to RTBC.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented

A lot of these services are ones I would never expect to be used outside the module. This issue doesn't really make that any more or less the case but wondering if we really want to do this for every service in core.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented

@catch the followup to this is #3295751: Autowire core services that do not require explicit configuration where we can start using autowiring for real across core, and don't have to specify exact arguments any more for a large number of service definitions. Even when the aliases are only used within a module, adding the alias allows autowiring within that module, or by anything that wants to inject the aliased service in contrib/custom code.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Oh I didn't think about within the module itself... makes sense then.

Committed/pushed to 11.x (10.2.x), thanks!

  • catch committed b83467ea on 11.x 
    Issue #3325557 by longwave, mondrake, kim.pepper: Enable more service...
mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented
Issue tags: +Service autowiring

Filed #3367416: [policy] Service autowiring - decide approach for multiple service implementations based on the same class and different arguments as a possible followup.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented

It needs follow-up according to #3354584-14: Deprecate TrustedCallbackInterface in favour of TrustedCallback attribute

some classes that used to implement interface now brings false positive

joachim’s picture

joachim CreditAttribution: joachim commented

This needs a CR to say that most core services need an interface alias.

The test assertion and test docs also needs more detail, otherwise adding a new service to core just gets you this confusing test failure and you're left doing git archeology to find this issue :(

Failed asserting that two arrays are identical.
--- Expected
+++ Actual
@@ @@
     'Drupal\Core\Asset\AssetQueryStringInterface' => 'asset.query_string'
     'Drupal\Core\Config\ConfigManagerInterface' => 'config.manager'
     'Drupal\Core\Config\ConfigFactoryInterface' => 'config.factory'
-    'Drupal\Core\Config\ConfigImporterFactory' => 'config.importer.factory'
     'Drupal\Core\Config\ConfigInstallerInterface' => 'config.installer'
     'Drupal\Core\Config\StorageCacheInterface' => 'config.storage'
     'Drupal\Core\Config\ImportStorageTransformer' => 'config.import_transformer'
andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Related issues: +#3392396: Improve AutowireTest to ignore TrustedCallbackInterface

We could use #3392396: Improve AutowireTest to ignore TrustedCallbackInterface for that