[Symfony 5] Replace app.root and site.path string services with container parameters

This is Symfony 5 compatibility.

There might be some mileage in doing this in Drupal 8 because we're going to trigger these deprecations an awful lot. These are very very low level services for Drupal.

Some other thoughts:

• app.root probably should be should be a container parameter - determined during container build. If you move the code you should have to do a container rebuild. The rebuild can determine the new app.root.
• site.path is determined by the request and should be part of that system - and so should be a service of some sort with the request stack injected. One massive problem we have is that site.path determines which modules can possibly by installed and where their code is and so this does things that the Symfony container is not really optimised for.
• At the moment both are determined by DrupalKernel - see \Drupal\Core\DrupalKernel::findSitePath and \Drupal\Core\DrupalKernel::guessApplicationRoot

I think we need to add the app.root to \Drupal\Core\DrupalKernel::getContainerCacheKey() to avoid the problems that @catch eludes to in #2328111-48: Replace most instances of the DRUPAL_ROOT constant with the app.root container parameter (thanks @chx for the archaeology).

app.root is generated by app.root.factory. The history is here: #2328111-102: Replace most instances of the DRUPAL_ROOT constant with the app.root container parameter

Re: #5:

At the moment both are determined by DrupalKernel - see \Drupal\Core\DrupalKernel::findSitePath and \Drupal\Core\DrupalKernel::guessApplicationRoot

The root path is actually determined by the front controller, which should inject it into the kernel.That's why it's a guess on the part of the kernel if it's not injected. See DrupalKernel::__construct().

In an ideal world, we'd deprecate guessApplicationRoot(), so that we require the root path to be injected, and it can be added to the container as a parameter during container build time. And the site path would be determined by a service provider that acts as a site router.

But the scope here is make things work with Symfony 5, so maybe we don't live in that ideal world. :-)

So core.services.yml says this:

  app.root:
    class: SplString
    factory: app.root.factory:get
    tags:
      - { name: parameter_service }
  app.root.factory:
    class: Drupal\Core\AppRootFactory
    arguments: ['@kernel']
    public: false

And then AppRootFactory::get() says this:

  /**
   * Gets the app root.
   *
   * @return string
   */
  public function get() {
    return $this->drupalKernel->getAppRoot();
  }

...which is not an SplString.

So if SplString satisfies our need for an object, then we should modify AppRootFactory::get() to return an SplString.

@mikelutz nice work this looks really good to me. Given that the container has different cache keys for different site roots I think we've got a good answer for the original concern as different site roots will use a different container.

Hmmm this raises an interesting concern - what happens if the containers can out-of-sync? Maybe we're doing this wrong. Maybe instead of adding it to the cache key we need to fail and point people at the rebuild process.

#20 yep that's a very clear expression of my concern.

I think going to rebuild.php is probably best. Automatically rebuilding could get a set in a very interesting state. Going to rebuild.php would at least allow someone to discover they are in this state.

This looks like a good minimal change to get rid of the deprecation to me.

Bumping priority again to critical, partly due to it fixing the test failure, partly because it'll be critical for Drupal 9 if not so much 8.

1. +++ b/core/lib/Drupal/Core/AppRootFactory.php
   @@ -30,7 +30,7 @@ public function __construct(DrupalKernelInterface $drupal_kernel) {
       * @return string
       */
      public function get() {
   -    return $this->drupalKernel->getAppRoot();
   +    return new StringableObject($this->drupalKernel->getAppRoot());
   

   I think we should update the return documentation

2. +++ b/core/lib/Drupal/Core/Extension/ExtensionDiscovery.php
   @@ -104,7 +104,7 @@ class ExtensionDiscovery {
      public function __construct($root, $use_file_cache = TRUE, $profile_directories = NULL, $site_path = NULL) {
   -    $this->root = $root;
   +    $this->root = (string) $root;
   

   Do we want to deprecate passing in a string $root and $site_path?

3. +++ b/core/lib/Drupal/Core/SitePathFactory.php
   @@ -31,7 +31,7 @@ public function __construct(DrupalKernelInterface $drupal_kernel) {
       *   The site path.
       */
      public function get() {
   -    return $this->drupalKernel->getSitePath();
   +    return new StringableObject($this->drupalKernel->getSitePath());
      }
   

   Need to update the return documentation

4. +++ b/core/lib/Drupal/Core/StringableObject.php
   @@ -0,0 +1,34 @@
   +namespace Drupal\Core;
   +
   +/**
   + * An object to hold a string value.
   + */
   +class StringableObject {
   

   I'd be tempted to put this in core/lib/Drupal/Core/DependencyInjection as this exists for the purposes of the container. In and of itself I don't think we want usage of this case to spread.

1. +++ b/core/lib/Drupal/Core/DependencyInjection/StringableServiceObject.php
   @@ -1,11 +1,11 @@
   -class StringableObject {
   +class StringableServiceObject {
   
   @@ -15,7 +15,7 @@ class StringableObject {
   -   * Constructs a StringableObject object.
   +   * Constructs a StringableServiceObject object.
   

   Maybe the word object is now redundant so maybe StringableService - I mean in some ways both service and object are pretty meaningless but I think here the word service does have some meaning as this object is only mean to be used where we need a service on the container that represents a string.

2. +++ b/core/lib/Drupal/Core/Extension/ExtensionDiscovery.php
   @@ -103,8 +103,8 @@ class ExtensionDiscovery {
   +  public function __construct(string $root, $use_file_cache = TRUE, array $profile_directories = NULL, string $site_path = NULL) {
   

   +1 for the primitive type hints. It makes the code quite elegant.

3. One thought though is that this is a breaking change where contrib / custom is expecting a string. Perhaps we should take the opportunity to create new service names and leave the old service string alone and deprecate them?

Closed old duplicate #2536012: The app.root and site.path services are invalid

FYI #3079810: core/help_topics directory does not work is adding another injection of app.root

1. +++ b/core/core.services.yml
   @@ -196,7 +196,7 @@ services:
   -    arguments: ['@app.root', '@site.path', '@cache_tags.invalidator.checksum']
   +    arguments: ['@application.root', '@site.sitepath', '@cache_tags.invalidator.checksum']
   
   @@ -706,8 +706,14 @@ services:
      app.root:
   -    class: SplString
   +    class: \SplString
        factory: app.root.factory:get
   +    deprecated: The "%service_id%" service is deprecated in drupal:8.8.0 and is removed from drupal:9.0.0. Use the application.root service instead. See https://www.drupal.org/node/3080612
   +    tags:
   +      - { name: parameter_service }
   +  application.root:
   +    class: Drupal\Core\DependencyInjection\StringableService
   +    factory: app.root.factory:getService
        tags:
          - { name: parameter_service }
   

   The issue summary does not explain the rationale for this rename?

2. +++ b/core/core.services.yml
   @@ -706,8 +706,14 @@ services:
      app.root.factory:
   

   🤔 It's odd to app.root.factory and application.root. Weirdly inconsistent?

3. +++ b/core/core.services.yml
   @@ -715,8 +721,14 @@ services:
      site.path:
   -    class: SplString
   +    class: \SplString
        factory: site.path.factory:get
   +    deprecated: The "%service_id%" service is deprecated in drupal:8.8.0 and is removed from drupal:9.0.0. Use the site.sitepath service instead. See https://www.drupal.org/node/3080612
   +    tags:
   +      - { name: parameter_service }
   +  site.sitepath:
   +    class: Drupal\Core\DependencyInjection\StringableService
   +    factory: site.path.factory:getService
        tags:
          - { name: parameter_service }
      site.path.factory:
   

   Same remarks here.

A lot of this is rehashes of the discussion on #2536012: The app.root and site.path services are invalid so linking it. The generic utility class over there makes a lot of sense as well instead of a one off. :shrug:

I'm wondering how to reconcile this issue with #3088246-20: [policy, no patch] How to handle Drupal 8.9.x deprecations (see also https://groups.drupal.org/node/535670). Is this something that's ok to punt to 9.1, or is it necessary to make 9.0 (and 8.9?) not invoke any Symfony 4.4 deprecations?

If it's necessary to make 9.0/8.9 not invoke Symfony 4.4 deprecations, is it reasonable to add the new services (and use them in core), but not yet mark the old ones as deprecated until 9.1?

Or, do we need to discuss making this (Critical) issue an exception to the policy in https://groups.drupal.org/node/535670?

We haven't got a deprecation message for this listed in DeprecationListenerTrait::getSkippedDeprecations() so it seems like resolving it wasn't a blocker for moving to Symfony 4.4. Apart from the issue in #2536012: The app.root and site.path services are invalid which is a general bug, I'm not sure exactly what the issue is here so we definitely need an issue summary update.

I do think we should consider making exceptions for Symfony 4.4/5.0 compatibility issues for 8.9.x/9.0.x deprecations so that there's less change later on, just not clear whether this really is one or not.

When I run a test on D9 that expects a certain deprecation message then I get this output:

Failed asserting that string matches format description.
--- Expected
+++ Actual
@@ @@
 @expectedDeprecation:
-%A  Drupal\system\Plugin\views\field\BulkForm is deprecated in drupal:8.5.0, will be removed before drupal:9.0.0. Use \Drupal\views\Plugin\views\field\BulkForm instead. See https://www.drupal.org/node/2916716.
-  The "Symfony\Component\HttpFoundation\File\MimeType\FileBinaryMimeTypeGuesser" class is deprecated since Symfony 4.3, use "Symfony\Component\Mime\FileBinaryMimeTypeGuesser" instead.
+  The "Symfony\Component\HttpFoundation\File\MimeType\MimeTypeGuesser" class is deprecated since Symfony 4.3, use "Symfony\Component\Mime\MimeTypes" instead.
+  The "Symfony\Component\HttpFoundation\File\MimeType\FileBinaryMimeTypeGuesser" class is deprecated since Symfony 4.3, use "Symfony\Component\Mime\FileBinaryMimeTypeGuesser" instead.
+  The "Symfony\Component\HttpFoundation\File\MimeType\FileinfoMimeTypeGuesser" class is deprecated since Symfony 4.3, use "Symfony\Component\Mime\FileinfoMimeTypeGuesser" instead.
+  The "Symfony\Bridge\PsrHttpMessage\Factory\DiactorosFactory" class is deprecated since symfony/psr-http-message-bridge 1.2, use PsrHttpFactory instead.
+  The "psr7.http_message_factory" service relies on the deprecated "Symfony\Bridge\PsrHttpMessage\Factory\DiactorosFactory" class. It should either be deprecated or its implementation upgraded.
+  Non-object services are deprecated since Symfony 4.4, please fix the "app.root" service which is of type "string" right now.
+  Non-object services are deprecated since Symfony 4.4, please fix the "app.root" service which is of type "string" right now.
+  Non-object services are deprecated since Symfony 4.4, please fix the "app.root" service which is of type "string" right now.
+  Non-object services are deprecated since Symfony 4.4, please fix the "app.root" service which is of type "string" right now.
+  Non-object services are deprecated since Symfony 4.4, please fix the "app.root" service which is of type "string" right now.
+  Non-object services are deprecated since Symfony 4.4, please fix the "app.root" service which is of type "string" right now.
+  Non-object services are deprecated since Symfony 4.4, please fix the "app.root" service which is of type "string" right now.
+  Non-object services are deprecated since Symfony 4.4, please fix the "app.root" service which is of type "string" right now.
+  Non-object services are deprecated since Symfony 4.4, please fix the "app.root" service which is of type "string" right now.
+  Non-object services are deprecated since Symfony 4.4, please fix the "app.root" service which is of type "string" right now.
+  Non-object services are deprecated since Symfony 4.4, please fix the "app.root" service which is of type "string" right now.
+  Non-object services are deprecated since Symfony 4.4, please fix the "site.path" service which is of type "string" right now.
+  Non-object services are deprecated since Symfony 4.4, please fix the "site.path" service which is of type "string" right now.
+  Non-object services are deprecated since Symfony 4.4, please fix the "site.path" service which is of type "string" right now.
+  Non-object services are deprecated since Symfony 4.4, please fix the "app.root" service which is of type "string" right now.
+  Non-object services are deprecated since Symfony 4.4, please fix the "app.root" service which is of type "string" right now.
+  Non-object services are deprecated since Symfony 4.4, please fix the "site.path" service which is of type "string" right now.
+  Non-object services are deprecated since Symfony 4.4, please fix the "site.path" service which is of type "string" right now.
+  Non-object services are deprecated since Symfony 4.4, please fix the "site.path" service which is of type "string" right now.
+  Non-object services are deprecated since Symfony 4.4, please fix the "site.path" service which is of type "string" right now.
+  Non-object services are deprecated since Symfony 4.4, please fix the "app.root" service which is of type "string" right now.
+  Non-object services are deprecated since Symfony 4.4, please fix the "app.root" service which is of type "string" right now.
+  Non-object services are deprecated since Symfony 4.4, please fix the "site.path" service which is of type "string" right now.
+  Non-object services are deprecated since Symfony 4.4, please fix the "site.path" service which is of type "string" right now.
+  Non-object services are deprecated since Symfony 4.4, please fix the "site.path" service which is of type "string" right now.
+  Non-object services are deprecated since Symfony 4.4, please fix the "site.path" service which is of type "string" right now.
+  Non-object services are deprecated since Symfony 4.4, please fix the "app.root" service which is of type "string" right now.
+  Non-object services are deprecated since Symfony 4.4, please fix the "app.root" service which is of type "string" right now.
+  Non-object services are deprecated since Symfony 4.4, please fix the "site.path" service which is of type "string" right now.
+  Non-object services are deprecated since Symfony 4.4, please fix the "site.path" service which is of type "string" right now.
+  Non-object services are deprecated since Symfony 4.4, please fix the "site.path" service which is of type "string" right now.
+  Non-object services are deprecated since Symfony 4.4, please fix the "site.path" service which is of type "string" right now.
+  Non-object services are deprecated since Symfony 4.4, please fix the "app.root" service which is of type "string" right now.
+  Non-object services are deprecated since Symfony 4.4, please fix the "app.root" service which is of type "string" right now.
+  Non-object services are deprecated since Symfony 4.4, please fix the "site.path" service which is of type "string" right now.
+  Non-object services are deprecated since Symfony 4.4, please fix the "site.path" service which is of type "string" right now.
+  Non-object services are deprecated since Symfony 4.4, please fix the "site.path" service which is of type "string" right now.
+  Non-object services are deprecated since Symfony 4.4, please fix the "site.path" service which is of type "string" right now.
+  Non-object services are deprecated since Symfony 4.4, please fix the "app.root" service which is of type "string" right now.
+  Non-object services are deprecated since Symfony 4.4, please fix the "app.root" service which is of type "string" right now.
+  Non-object services are deprecated since Symfony 4.4, please fix the "site.path" service which is of type "string" right now.
+  Non-object services are deprecated since Symfony 4.4, please fix the "site.path" service which is of type "string" right now.
+  Non-object services are deprecated since Symfony 4.4, please fix the "site.path" service which is of type "string" right now.
+  Non-object services are deprecated since Symfony 4.4, please fix the "site.path" service which is of type "string" right now.

Took me a while to find them too but the problem is that they are dynamic, so we put it in \Drupal\Tests\Listeners\DeprecationListenerTrait::isDeprecationSkipped.

But I'm not sure if we should be doing that, as this suppresses it for all other string services too that custom/contrib code might add?

Starting with a reroll, I assume that this is a D9 issue now as there's no reason to fix this in D8 anymore, as we'll never update to symfony 4 there.

While rerolling, I did remove a large chunk (mostly comment) in \Drupal\KernelTests\Core\Theme\BaseThemeDefaultDeprecationTest that apparently isn't needed anymore to pass the test.

And of course a few changes were for code that is now removed in D9.

I also removed the skipped deprecation messages now and fixed a few new instances that use app.root. Still quite a bit of cleanup to do.

That was an accidental leftover where I tested that the deprecation message is gone.

So here's one idea that builds on #3111008: Use native Symfony YamlLoader + Config and brings in the Symfony expression language.

However I have a few concerns about this - namely the additional complexity. I thought I'd leave it here in case it inspires any thoughts... I'm not a huge fan of StringableService - it seems way too parameter like for my taste.

This patch will fail at least a few tests but Drupal installs and at least 1 functional test passes.

So #54 is possible :) - but as I said we're adding a tonne of complexity and at least the first get to each version of the expression needs work. It's cached afterwards in a static array cache - almost certainly possible to store the parsed expression in our db dumped container so that would save this work.

Here's a simpler solution using parameters again like #23 but importantly without the downsides. Because it sets the parameters on the compiled container dynamically prior to re-instantiating it.

It seems there's an instance where we have a container without a kernel - interesting. But not the fault of this patch...

So delved into why

we have a container without a kernel

- see fails in #57 and the fix n #58. It's because we have extension objects stored in the FileCache and we use this when compiling the container. So right now in HEAD when compiling the container we fallback to DRUPAL_ROOT in \Drupal\Core\Extension\Extension::__wakeup() when it is called during initialising service providers as part of the very very early code in the container compilation. This occurs even before the container as the kernel service set! The funny thing is that we could cache these objects with the app root here because it is stored in the file cache and therefore cached by the absolute path. However we can't assume that all caches that store extension objects are keyed by something that represents the absolute path.

Since the above is all happening in HEAD atm - I've changed change \Drupal\Core\Extension\Extension::__wakeup() in the patch to behave in the same way - ie. falling back to DRUPAL_ROOT at this point. I've also added docs so that we eventually try to remove \Drupal we'll be aware that we have to handle this in some way.

The comment in the code in #62 is not my best. Trying again.

I've opened #3116858: Don't cache whole extension objects in FileCache to address the Extension object relying on DRUPAL_ROOT issue.

Updating the issue summary.

Also I think we should consider backporting without the deprecations to 8.9.x. To that end we should reimplement the services to return the container parameter.

Using the parameters makes sense to me. CR looks good. One small nit (can be fixed on commit):

+++ b/core/core.services.yml
@@ -1,4 +1,8 @@
+  # The app.root and site.path parameters are dynamically by

"are dynamically provided" or something like that. Seems to be missing some words.

Fixed the comment and went into a bit more detail why it's worth listed them there.

+1 on the comment updates

Committed/pushed to 9.0.x, thanks!

Removing the deprecations for 8.9.x seems good, not 100% sure on the conversions.

Moving to needs work for the 8.9.x backport.

Filling out the 8.9.x and 8.8.x plans after discussing with @catch a bit.

I think we have two decisions to make:

• Whether to convert the service usages to the params in 8.9.x - I'm in favour of that because it keeps 8.9.x and 9.0.0 closer making patches easier.
• What to do about 8.8.x - I think we should consider adding the parameters there to make it easier for contrib to support 8.8.x -> 9.0.0.

Looks it missing post update hook to cause rebuild of container

@andypost the container is keyed by \Drupal::VERSION - we don't need post updates to do that.

It still for backport, second option from #75 looks the most promissing

Working on this.

Here I have added reroll of patch #69 for Drupal 8.9.x.

+++ b/core/core.services.yml
@@ -717,23 +722,27 @@ services:
-    class: SplString
+    class: \SplString
     factory: app.root.factory:get
+    deprecated: The "%service_id%" 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
     tags:
       - { name: parameter_service }
   app.root.factory:
     class: Drupal\Core\AppRootFactory
     arguments: ['@kernel']
     public: false
+    deprecated: The "%service_id%" 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
   site.path:
-    class: SplString
+    class: \SplString
     factory: site.path.factory:get
+    deprecated: The "%service_id%" service is deprecated in drupal:9.0.0 and is removed from drupal:10.0.0. Use the site.path parameter instead. See https://www.drupal.org/node/3080612
     tags:
       - { name: parameter_service }
   site.path.factory:
     class: Drupal\Core\SitePathFactory
     arguments: ['@kernel']
     public: false
+    deprecated: The "%service_id%" service is deprecated in drupal:9.0.0 and is removed from drupal:10.0.0. Use the site.path parameter instead. See https://www.drupal.org/node/3080612

We should not be deprecating these service in D8. None of this should change.

Also I think that now we have 8.9.0 what we should backport has changed. I think we need to follow the plan for 8.8.x. I.e. I think we should consider adding the parameters there to make it easier for contrib to support 8.8.x -> 9.0.0.

We shouldn't change any existing services. All we need to ensure is that the container has the new parameters and they are correct.

Note given that the services still exist in D9 the other option is to close this issue. If you want to be be D8 and D9 compatible use the deprecated services.

Here I have made changes as suggested in comment #83.

We still need to address comment #84.

Retitling to make it clearer when scanning the issue queue that we're only discussing the backport here.

Technically it's too late to add the new services to 8.9.x, but we might want to make an exception and backport anyway.

Fixing test, Please review.

Fixed test, Please review.

Fixed test, Please review.

If we are to backport this now we can't make all these changes, we can only add the new services as in #84. But at this stage, perhaps this is just "won't fix" for 8.9.x and can be closed as fixed in D9?

At this point I think we should just mark this fixed against 9.0.x - the window for supporting both 8.9.x and 9.1.x is already half gone since this was originally committed.

Published the CR at https://www.drupal.org/node/3080612.

FYI: this is how you can be compatible with Drupal 8 and 9 while not triggering any Drupal 10 deprecation notices (i.e. be compatible with Drupal 8, 9 and 10):

diff --git a/cdn.services.yml b/cdn.services.yml
index da9db27..c12756d 100644
--- a/cdn.services.yml
+++ b/cdn.services.yml
@@ -5,7 +5,7 @@ services:
 
   cdn.file_url_generator:
     class: Drupal\cdn\File\FileUrlGenerator
-    arguments: ['@app.root', '@stream_wrapper_manager', '@request_stack', '@private_key', '@cdn.settings']
+    arguments: ['%app.root%', '@stream_wrapper_manager', '@request_stack', '@private_key', '@cdn.settings']
 
   # Event subscribers.
   cdn.config_subscriber:
diff --git a/src/CdnServiceProvider.php b/src/CdnServiceProvider.php
index aec1a4b..365b133 100644
--- a/src/CdnServiceProvider.php
+++ b/src/CdnServiceProvider.php
@@ -29,6 +29,12 @@ class CdnServiceProvider implements ServiceProviderInterface {
         //   banned, without having to run this middleware.
         ->addTag('http_middleware', ['priority' => 230]);
     }
+    // @todo Delete this when dropping Drupal 8 support in https://www.drupal.org/project/cdn/issues/3103682.
+    if (version_compare(\Drupal::VERSION, '9.0', '<')) {
+      // @see https://www.drupal.org/project/drupal/issues/3074585
+      $container->getDefinition('cdn.file_url_generator')
+        ->setArgument(0, '@app.root');
+    }
   }
 
   /**

Also added this to the change record.

(Developed in #3228162: Fix Drupal 9-only deprecation notices while retaining Drupal 8 compatibility.)