[Symfony 6] Refactor the "update.root" service to return an object not a string

+++ b/core/lib/Drupal/Component/DependencyInjection/Container.php
@@ -315,14 +315,14 @@ public function set($id, $service) {
-  public function getParameter($name) {
+  public function getParameter($name): array|bool|string|int|float|null {

this is D10 (php 8) only territory

TypeError: Drupal\Component\DependencyInjection\Container::get(): Return value must be of type ?object, string returned

I started taking a look at this. The update.root service is being used. Updating the title here accordingly.

The following files in core/modules/update all use this service:
./update.authorize.inc
./src/Form/UpdateManagerInstall.php
./src/Form/UpdateReady.php

The "service" is a shim to return the root path that the update manager should use for installing new / updated packages. It's actually something to make update manager more testable. Since the meat of the "service" is this:

  public function get() {
    // Normally the Update Manager's root path is the same as the app root (the
    // directory in which the Drupal site is installed).
    $root_path = $this->drupalKernel->getAppRoot();

    // When running in a test site, change the root path to be the testing site
    // directory. This ensures that it will always be writable by the webserver
    // (thereby allowing the actual extraction and installation of projects by
    // the Update Manager to be tested) and also ensures that new project files
    // added there won't be visible to the parent site and will be properly
    // cleaned up once the test finishes running. This is done here (rather
    // than having the tests enable a module which overrides the update root
    // factory service) to ensure that the parent site is automatically kept
    // clean without relying on test authors to take any explicit steps. See
    // also \Drupal\update\Tests\Functional\UpdateTestBase::setUp().
    if (DRUPAL_TEST_IN_CHILD_SITE) {
      $kernel = $this->drupalKernel;
      $request = $this->requestStack->getCurrentRequest();
      $root_path .= '/' . $kernel::findSitePath($request);
    }

    return $root_path;
  }

I can't believe we have real BC implications to worry about here. I can't imagine real sites are trying to override this "service" to customize Update Manager behavior. I think we just need to reconsider how this should work to be more Symfony 6 friendly. We can probably just change it so that get() returns the service and the service provides a new getRoot() method or whatever.

In #3074585: [Symfony 5] Replace app.root and site.path string services with container parameters we converted the app.root service (which returned a similar string) to a container parameter, maybe we can do the same for this service.

edit: @daffie already suggested this :)

Looking at this again I think this does need to be a simple service that returns a string, as it requires the request stack to be injected; a container parameter is set at container build time and the "current request" isn't available in that context.

Also agree with #12 that this is test-only behaviour, the existing service should probably have been tagged @internal and I suggest we do the same for any new service we provide here.

Maybe you could use service provider to provide required service depending on php version, so old one could be removed in D10

or as we used to for similar case in #3074585: [Symfony 5] Replace app.root and site.path string services with container parameters

ide_helper has no release or usage (and is a developer module). patchinfo does have stable releases and usage, but the update.root usage is only in its test coverage.

Given that I think opening an issue against both modules + a CR might be enough here.

On the patch itself:

+++ b/core/modules/update/tests/src/Functional/UpdateTestBase.php
@@ -62,15 +69,13 @@ protected function setUp() {
     // test child site.
     $request = \Drupal::request();
-    $update_root = $this->container->get('update.root') . '/' . DrupalKernel::findSitePath($request);
-    $this->container->set('update.root', $update_root);
-    \Drupal::setContainer($this->container);

Nice that setting this back on the container isn't necessary.

#3232095-19: [Symfony 6] Refactor the "update.root" service to return an object not a string - are we able to set container parameters from a module? If so that might work as an alternative approach but I can't think of any examples. The two we changed there are hard-coded in DrupalKernel.

> are we able to set container parameters from a module?

Yes, we can add a service provider to update.module that implements ServiceProviderInterface/ServiceModifierInterface. But the problem is this code:

    if (DRUPAL_TEST_IN_CHILD_SITE) {
      $kernel = $this->drupalKernel;
      $request = $this->requestStack->getCurrentRequest();
      $root_path .= '/' . $kernel::findSitePath($request);
    }

When the container is being rebuilt are we sure the constant will be set and the "current request" will contain the correct site path? This is much, much earlier than the current factory which does these checks at runtime.

edit: if we can guarantee that DRUPAL_TEST_IN_CHILD_SITE will be set then it would perhaps make sense for that to be a container parameter too!

Filed issues and patches, I think it ready according to #20

- used in testing code and probably could be removed #3239550: Fix update.root for 9.3.x core and PHP 8.1
- initial patch #3239551: Fix update.root for 9.3.x core and PHP 8.1

RTBC++

Thanks for all the work here. I haven’t reviewed closely, but in principle this all sounds good. Meanwhile, giving this a slightly more clear title.

diff --git a/core/lib/Drupal/Component/DependencyInjection/Container.php b/core/lib/Drupal/Component/DependencyInjection/Container.php
index 41684d3e70..560d9149cc 100644
--- a/core/lib/Drupal/Component/DependencyInjection/Container.php
+++ b/core/lib/Drupal/Component/DependencyInjection/Container.php
@@ -127,7 +127,7 @@ public function __construct(array $container_definition = []) {
   /**
    * {@inheritdoc}
    */
-  public function get($id, $invalid_behavior = ContainerInterface::EXCEPTION_ON_INVALID_REFERENCE) {
+  public function get($id, $invalid_behavior = ContainerInterface::EXCEPTION_ON_INVALID_REFERENCE): ?object {
     if ($this->hasParameter('_deprecated_service_list')) {
       if ($deprecation = $this->getParameter('_deprecated_service_list')[$id] ?? '') {
         @trigger_error($deprecation, E_USER_DEPRECATED);
@@ -160,7 +160,7 @@ public function get($id, $invalid_behavior = ContainerInterface::EXCEPTION_ON_IN
     // is used, the actual wanted behavior is to re-try getting the service at a
     // later point.
     if (!$definition) {
-      return;
+      return NULL;
     }

Shouldn't the container changes happen in their own issue?

Yep there's already an issue open, we can always open a 9.x issue for anything we want to do from here, but don't think we should combine the changes in this one.

#3238485: [Symfony 6] Add return type hints to the class methods of Drupal\Component\DependencyInjection\Container.

I think this is the simplest thing we can do here to solve the problem at hand.

I tried switching the service to a container parameter in a service provider, but it turns out that in a test the container is built in BTB at module install time, not inside the site under test - so the container builder is unaware of where the container will be used, the DRUPAL_TEST_IN_CHILD_SITE constant is not set, and we have no other way to detect this situation that I can see. As @daffie found earlier we also can't modify the parameter inside the container once it's built, so I think the simplest solution is a small service that returns the correct path at runtime depending on whether it is inside a real site or a site under test, which is what we have here.

Agree with @catch in #20 that we can probably remove the existing update.root "service" without deprecation, as only one module really uses it (ide_helper doesn't count to me), and it's a niche feature that is extremely unlikely to be used by custom code.

Circling back to look at this more closely. Agreed this looks good. Thanks @longwave for trying the other approach and confirming it doesn't work. Saving credits for everyone who participated so far, since everyone has contributed something of substance to getting this done.

I only found one micro-nit. Uploading here as a patch/interdiff, and leaving RTBC. Hope I'm not struck by lightning for that. 🤞😉

1. +++ b/core/modules/update/src/UpdateRoot.php
   @@ -38,6 +45,16 @@ public function __construct(DrupalKernelInterface $drupal_kernel, RequestStack $
   +  public function set(string $update_root): void {
   +    $this->updateRoot = $update_root;
   +  }
   

   usually setters return $this but it's used only one so fine

2. +++ b/core/modules/update/src/UpdateRoot.php
   @@ -46,6 +63,11 @@ public function __construct(DrupalKernelInterface $drupal_kernel, RequestStack $
   +    // Return the $updateRoot when it is set.
   +    if (!empty($this->updateRoot)) {
   

   wondering why not set is equal to empty string

Thanks for the review, @andypost. Re: #32:

1. Right. I don't think we care here. 😉
2. I don't understand your comment. That's an early return if we already know the answer. Otherwise, we compute the right thing and return it later.

That said, seems we should add the explicit return type for get() since it always returns a string, never NULL.

+++ b/core/modules/update/src/UpdateRoot.php
@@ -45,7 +62,12 @@ public function __construct(DrupalKernelInterface $drupal_kernel, RequestStack $
+    // Return the $updateRoot when it is set.
+    if (!empty($this->updateRoot)) {

I mean if (isset($var)) is more readable and fast

Ahh, gotcha. Sure, why not? 😉

I think it ready)

I had a thought that we could try using the magic __toString() method instead of get(), which would mean to callers that this doesn't require a code change, but that it still satisfies the container in that the service is an object. This works for two of the update tests locally, let's see how the bot gets on.

Agreed with @daffie in Slack that if this passes here we should also test against SF5.4 to ensure there are no issues.

That's promising! I like it.

How do we test on SF5.4? Is that manual at this point or is there a way to do that via DrupalCI?

@dww The best way is by applying the latest patch in #3161889: [META] Symfony 6 compatibility but Symfony's future return type deprecations are currently causing a lot of unwanted noise and false positives which makes the test results hard to read.

There were no update module failures in https://www.drupal.org/pift-ci-job/2228443 which was run from https://www.drupal.org/files/issues/2021-11-05/3161889-186.patch - which is various Symfony 5.4 compatibility fixes plus #37 from this issue.

This looks good. Wondered whether we want to keep the __toString() around permanently, but given this is mostly internal to update module anyway I think it's OK.

Committed f96b7fb and pushed to 9.4.x. Thanks!

Is this committed to D10.0.x?

Here is an example where the same AdminFunctionalityTest passes the D9.4.x test and fails the D10.0.x test with the error message:

Exception: TypeError: Drupal\Component\DependencyInjection\Container::get(): Return value must be of type ?object, string returned

#3257627: Add the AdminFunctionality tests

If this post is out-of-topic here please feel free to reply at the related topic and I will delete this message afterwards.

Published the change record.