Problem/Motivation

Drupal has restricted service IDs/names to be lowercased since before Drupal 8: #2498293: Only allow lowercase service and parameter names.

Since then, Symfony has added "autowiring" of services. We're working to adopt that in Drupal: #3295751: Autowire core services that do not require explicit configuration, #3049525: Enable service autowiring by adding interface aliases to core service definitions, #3228629: [meta] Bring dependency injection more in line with upstream Symfony component.

But … that restriction from 2015 prevents this adoption.

Worse, that restriction is implemented inconsistently! Quoting @Wim Leers from #2503567-51: Only allow lowercase service and parameter names [part 2]:

This is showing up explicitly in #3314137: Make Automatic Updates Drupal 10-compatible since #3284422: [META] Symfony 6.2 compatibility due to changes in Symfony 6.2's FileLoader — see #3314137-27: Make Automatic Updates Drupal 10-compatible and #3314137-28: Make Automatic Updates Drupal 10-compatible for details. In #3314137-29: Make Automatic Updates Drupal 10-compatible I discovered that even Drupal core's existing autowiring test technically violates 

  public function register($id, $class = NULL): Definition {
    if (strtolower($id) !== $id) {
      throw new \InvalidArgumentException("Service ID names must be lowercase: $id");
    }

… by somehow bypassing it thanks to ::setDefinition(), which doesn't have that same check:

😬

So I agree with @longwave in #49. Closing this. See y'all in #3049525: Enable service autowiring by adding interface aliases to core service definitions.

The only reason this hasn't come up in the limited autowiring support we have today is because that has used \Drupal\Core\DependencyInjection\ContainerBuilder::setAlias(), which … does not have this restriction — an oversight from #2498293: Only allow lowercase service and parameter names 😅

Proposed resolution

Allow uppercase service IDs.

Remaining tasks

TBD

User interface changes

None.

API changes

None.

Data model changes

None.

Release notes snippet

TBD

CommentFileSizeAuthor
#8 3320315-8.patch2.36 KBWim Leers
#8 interdiff.txt1.14 KBWim Leers
#6 3320315-5.patch3.2 KBAnchal_gupta
#6 interdiff-3320315-2_5.txt737 bytesAnchal_gupta
#2 3320315-2.patch2.98 KBWim Leers
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers created an issue. See original summary.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Active » Needs review
Issue tags: +Needs tests
FileSize
3320315-2.patch2.98 KB

Let's see how this does.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Parent issue: » #3228629: [meta] Bring dependency injection more in line with upstream Symfony component
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Needs review » Needs work 
+++ b/core/lib/Drupal/Core/DependencyInjection/ContainerBuilder.php
@@ -29,31 +29,10 @@ public function __construct(ParameterBagInterface $parameterBag = NULL) {
-    SymfonyContainer::set($id, $service);

Removing this is not correct. This is skipping the parent and calling the parent of parent....

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Issue tags: +Needs change record, +Needs change record updates

We should write a short change record and also update https://www.drupal.org/node/2498887 to note that it is now obsolete.

Anchal_gupta’s picture

Anchal_gupta CreditAttribution: Anchal_gupta at Srijan | A Material+ Company for Drupal India Association commented
FileSize
interdiff-3320315-2_5.txt737 bytes
3320315-5.patch3.2 KB

Fixed CS error. Please review it

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented

As per #4 - fixing the CS error is not enough... in fact we need to add the use statement back in and only remove the case check...

The code should like this:

  /**
   * Overrides Symfony\Component\DependencyInjection\ContainerBuilder::set().
   *
   * Drupal's container builder can be used at runtime after compilation, so we
   * override Symfony's ContainerBuilder's restriction on setting services in a
   * frozen builder.
   *
   * @todo Restrict this to synthetic services only. Ideally, the upstream
   *   ContainerBuilder class should be fixed to allow setting synthetic
   *   services in a frozen builder.
   */
  public function set($id, $service) {
    SymfonyContainer::set($id, $service);
  }
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs work » Needs review
FileSize
interdiff.txt1.14 KB
3320315-8.patch2.36 KB

#4: ah, so that's why it didn't use parent::! 😄

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Issue summary: View changes
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Issue tags: +Contributed project blocker

Without this, I had to add this insane (and unmaintainable) work-around to make https://www.drupal.org/project/automatic_updates work on Drupal 10/Symfony 6.2: https://git.drupalcode.org/project/automatic_updates/-/merge_requests/52...

Also, yay, #8 is green! I marked it Needs tests — to assert we can now add uppercase service IDs. But … is that really worth testing? 🤔

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Issue tags: -Needs tests

I don't think we should add tests - we're removing the code and soon enough we'll have uppercase services I guess.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Issue tags: -Needs change record, -Needs change record updates

#11: 👍

#5:

  1. Change record created: https://www.drupal.org/node/3320387
  2. Change record updated: https://www.drupal.org/node/2498887 (changes: https://www.drupal.org/node/2498887/revisions/view/8532545/12855428)
longwave’s picture

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

Looks good to me. I fixed one of the links in the change record but otherwise everything is great, we are only removing code here :)

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed a5f4685c6a to 10.1.x and 95e742555b to 10.0.x and 23a9ff75be to 9.5.x. Thanks!

Backported this to 9.5.x to make making D9/10 modules easier.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

Woah!!! That was incredibly fast :D Very grateful for that, makes #3314137-33: Make Automatic Updates Drupal 10-compatible far less of a dystopian satirical nightmare 😆

  • alexpott committed a5f4685 on 10.1.x 
    Issue #3320315 by Wim Leers, Anchal_gupta, alexpott, longwave: Allow...

  • alexpott committed 95e7425 on 10.0.x 
    Issue #3320315 by Wim Leers, Anchal_gupta, alexpott, longwave: Allow...

  • alexpott committed 23a9ff7 on 9.5.x 
    Issue #3320315 by Wim Leers, Anchal_gupta, alexpott, longwave: Allow...

Status: Fixed » Closed (fixed)

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