Problem/Motivation

#2531564: Fix leaky and brittle container serialization solution only handles services that are objects, but Drupal 9 has some (deprecated) services that are strings, that results in a type error.

Steps to reproduce

The quickest way to replicate is to run this code in 9.5.x using drush php. The first statement just generates a deprecation warning, but the second generates an error.

>>> \Drupal::getContainer()->get('app.root')
<warning>PHP Deprecated:  The "app.root" service is deprecated in drupal:9.0.0 and is removed from drupal:10.0.0. Use the app.root parameter instead. See https://www.drupal.org/node/3080612 in /home/mortenson/repos/drupal/core/lib/Drupal/Component/DependencyInjection/Container.php on line 134</warning>
=> "/home/mortenson/repos/drupal"
>>> \Drupal::getContainer()->getServiceIdMappings()
<warning>PHP Deprecated:  The "app.root" service is deprecated in drupal:9.0.0 and is removed from drupal:10.0.0. Use the app.root parameter instead. See https://www.drupal.org/node/3080612 in /home/mortenson/repos/drupal/core/lib/Drupal/Component/DependencyInjection/Container.php on line 134</warning>
TypeError: Argument 1 passed to Drupal\Component\DependencyInjection\Container::generateServiceIdHash() must be an object, string given, called in /home/mortenson/repos/drupal/core/lib/Drupal/Component/DependencyInjection/ServiceIdHashTrait.php on line 19

Proposed resolution

Handle the string services when serializing too.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#13 3310271-13.patch1.75 KBcatch
#13 3310271-test-only.patch936 bytescatch
#10 3310271-10.patch859 byteslongwave
#7 3310271-7.patch2.55 KBlongwave
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

catch created an issue. See original summary.

bradjones1’s picture

bradjones1
Primary language English
CreditAttribution: bradjones1 at Not Vanilla, Inc. commented

E.g., app.root, see #2531564-261: Fix leaky and brittle container serialization solution

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Title: Container serialiazation must handle string services in 9.x » Container serialization must handle string services in 9.x
andypost’s picture

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

It's one more reason to not use weakmap

benjifisher credited samuel.mortenson.

benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
CreditAttribution: benjifisher as a volunteer and at FRUITION commented
Issue summary: View changes

I am copying the steps to reproduce from #2531564-261: Fix leaky and brittle container serialization solution to the issue summary and crediting @samuel.mortenson.

I tested the instructions and got the same result.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Active » Needs review
FileSize
3310271-7.patch2.55 KB

Can we just do something like this in 9.5.x? Services must be objects in Drupal 10, so we don't need the fix there.

longwave’s picture

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

Although actually I doubt serializing the app.root string would have been correctly re-injected before, so maybe we just skip the case when the service isn't an object?

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Status: Needs review » Needs work

Yes, lets not try to handle strings in the trait. That's not really needed from a performance aspect either and while it could get out of sync, that's unchanged to HEAD.

that should also fix the composer install fatal error.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs work » Needs review
FileSize
3310271-10.patch859 bytes

This solves the case in the OP:

>>> \Drupal::service('app.root');
=> "/var/www/html/drupal"

>>> \Drupal::getContainer()->getServiceIdMappings();
=> [
     "8b92ee7b6a76d1eb0b1558e676ffea4b3cfb9acdbe28a29a3dd58149bcf3071d" => "drush.console.services",
     "a702cffdfcc6b3058830b35123d38bead8c6810207d017d14ca9528fe21a2f61" => "drush.command.services",
     "534d7075e7516f42f23044e79fb18bfa54a1d43d541e845d63bc641634b811df" => "drush.command_info_alterer.services",
     "009a9be74a7b397e4bfa2214f19c0569aa528369b23dbbef87efdd1f3395bef1" => "class_loader",
     "c6de8d012ab79e086a59abbae070b069b9b88ef3d6e4e984f1cf008e3c8fb042" => "kernel",
...
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Issue tags: +9.5.0 update

Tagging to indicate this is a regression in 9.5.0.

benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
CreditAttribution: benjifisher as a volunteer and at FRUITION commented
Issue summary: View changes

I applied the patch from #10 and repeated the steps in the issue description. There are no errors, just the deprecation warning:

Psy Shell v0.10.12 (PHP 8.1.6 — cli) by Justin Hileman
Drush Site-Install (Drupal 9.5.0-dev)
>>> \Drupal::getContainer()->get('app.root')
<warning>PHP Deprecated:  The "app.root" service is deprecated in drupal:9.0.0 and is removed from drupal:10.0.0. Use the app.root parameter instead. See https://www.drupal.org/node/3080612 in /var/www/html/core/lib/Drupal/Component/DependencyInjection/Container.php on line 134</warning>
=> "/var/www/html"
>>> \Drupal::getContainer()->getServiceIdMappings()
<warning>PHP Deprecated:  The "app.root" service is deprecated in drupal:9.0.0 and is removed from drupal:10.0.0. Use the app.root parameter instead. See https://www.drupal.org/node/3080612 in /var/www/html/core/lib/Drupal/Component/DependencyInjection/Container.php on line 134</warning>
<warning>PHP Deprecated:  The "app.root.factory" service is deprecated in drupal:9.0.0 and is removed from drupal:10.0.0. Use the app.root parameter instead. See https://www.drupal.org/node/3080612 in /var/www/html/core/lib/Drupal/Component/DependencyInjection/Container.php on line 134</warning>
=> [
     "eec038f97ae626a0cb2be310045ccbfb6a8788f5e347817faf97c96f66f21831" => "drush.console.services",
     "fca9cf0f36d6e2cd77b2ff735bf097c29547feba51ac67a5c50ac6d9eabc7a9b" => "drush.command.services",
     "107c276d641655a3b00b3c4928dfb100dfbd30b156a3f0218f75256855eff555" => "drush.command_info_alterer.services",
     "6e7c95bf96e707dd27ef8cf0cd8be9ac5f8c226174a567c46178d1536c410d5f" => "class_loader",
...

I do notice some odd things, but I am not sure whether any of these are problems.

  1. app.root.factory, but not app.root, is one of the mappings.
  2. \Drupal::getContainer()->get('app.root.factory') generates an error unless I do \Drupal::getContainer()->get('app.root') first.

The second point is not a new problem: I get the same behavior with 9.5.x and 9.4.5.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
FileSize
3310271-test-only.patch936 bytes
3310271-13.patch1.75 KB

app.root.factory is a class so it's probably fine to serialize theoretically. It probably shouldn't be injected anything but it's already deprecated so I think that much is probably covered by the existing deprecation.

Added a test based on the OP.

The last submitted patch, 13: 3310271-test-only.patch, failed testing. View results

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Status: Needs review » Reviewed & tested by the community

I find it ready

neclimdul’s picture

neclimdul
He/Him
CreditAttribution: neclimdul as a volunteer commented

Shoot sorry! I went on issue queue holiday. I remember seeing this at some point and thought we'd caught it but in a monster issue like that a subtle thing like this can easily look like a bug/cruft and get lost in a reroll/refactor. :(

Aweseome work! Agreed this looks like good to go.

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 77d7fb6 and pushed to 9.5.x. Thanks!

This fix looks good and the test coverage complete.

  • alexpott committed 77d7fb6 on 9.5.x 
    Issue #3310271 by catch, longwave, benjifisher, samuel.mortenson:...
maxstarkenburg’s picture

maxstarkenburg
he/him
CreditAttribution: maxstarkenburg at TEN7 commented

Thanks, I was wondering why a post-upgrade drush updb was erroring (before also realizing that the upgrade unexpectedly jumped me to 9.5.0-beta1, though that's a separate composer issue). Can confirm that having the patch in place would prevent the updb error.

benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
CreditAttribution: benjifisher as a volunteer and at FRUITION commented

Status: Fixed » Closed (fixed)

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