Remove usages of deprecated path alias services and clear "DeprecationListenerTrait" entries

1. +++ b/core/core.api.php
   @@ -761,7 +761,7 @@
     *
     * A typical service definition in a *.services.yml file looks like this:
     * @code
   - * path.alias_manager:
   + * path_alias.manager:
     *   class: Drupal\path_alias\AliasManager
     *   arguments: ['@path.crud', '@path_alias.whitelist', '@language_manager']
   

   this is an arbitrary example but now it's one that is no longer in core.services.yml. In a way, I suppose that's better because such an example is closer to what people looking at that documentation actually want to do ;)

   That said, it has path.crud as an argument, which hasn't existed for, uhm, 6 years :)

2. +++ b/core/core.services.yml
   @@ -1218,7 +1218,7 @@ services:
      path_subscriber:
        class: Drupal\Core\EventSubscriber\PathSubscriber
   -    arguments: ['@path.alias_manager', '@path.current']
   +    arguments: ['@path_alias.manager', '@path.current']
   

   doesn't really matter if the deprecated service or not, but I guess it is currently identical and would result in fewer (misleading) deprecation messages if you'd only access that.

3. +++ b/core/modules/path/src/PathAliasListBuilder.php
   @@ -63,7 +63,7 @@ class PathAliasListBuilder extends EntityListBuilder {
       *   The language manager.
   -   * @param \Drupal\Core\Path\AliasManagerInterface $alias_manager
   +   * @param \Drupal\path_alias\AliasManagerInterface $alias_manager
       *   The path alias manager.
   

   this is new in 8.8, so if we can still get it in, I suppose we don't have to worry about BC :)

4. +++ b/core/modules/system/src/Form/SiteInformationForm.php
   @@ -64,7 +64,7 @@ public function __construct(ConfigFactoryInterface $config_factory, AliasManager
          $container->get('config.factory'),
   -      $container->get('path.alias_manager'),
   +      $container->get('path_alias.manager'),
          $container->get('path.validator'),
   

   Ok, so you're currently only updating the service but not the interface for this form and RequestPath (at least with this one, a subclassed constructor is imaginable.. found one but that doesn't change the constructor: http://grep.xnddx.ru/node/29452542).

   What we could *maybe* still get into 8.8 beta would be removing the type hint checking if it implements the new interface, and if not, then we'd do a @trigger_error?

5. +++ b/core/modules/views/src/Plugin/views/argument_default/Raw.php
   @@ -64,7 +64,7 @@ public static function create(ContainerInterface $container, array $configuratio
          $plugin_id,
          $plugin_definition,
   -      $container->get('path.alias_manager'),
   +      $container->get('path_alias.manager'),
          $container->get('path.current')
   

   same.

6. +++ b/core/modules/workspaces/workspaces.services.yml
   @@ -40,7 +40,7 @@ services:
          - { name: 'event_subscriber' }
      workspaces.workspace_subscriber:
        class: Drupal\workspaces\EventSubscriber\WorkspaceRequestSubscriber
   -    arguments: ['@path.alias_manager', '@path.current', '@router.route_provider', '@workspaces.manager']
   +    arguments: ['@path_alias.manager', '@path.current', '@router.route_provider', '@workspaces.manager']
        tags:
   

   workspaces is/was still experimental and this class is new, so we can update the type hint of this class.

7. +++ b/core/tests/Drupal/Tests/Core/DependencyInjection/Compiler/ProxyServicesPassTest.php
   @@ -67,7 +68,7 @@ public function testContainerWithLazyServices() {
      public function testContainerWithLazyServicesWithoutProxyClass() {
        $container = new ContainerBuilder();
   -    $container->register('alias_whitelist', 'Drupal\Core\Path\AliasWhitelist')
   +    $container->register('alias_whitelist', AliasWhitelist::class)
          ->setLazy(TRUE);
   

   should a test in core/tests rely on a path_alias service or should we use something else for this test? not sure.

Patch #5 has passed the test so I am moving it to needs review.

Patch looks good to me. Can we still backport this to 8.8.x?

+++ b/core/modules/system/src/Form/SiteInformationForm.php
@@ -43,16 +44,26 @@ class SiteInformationForm extends ConfigFormBase {
 
+    if (!$alias_manager instanceof AliasManagerInterface) {
+      if ($alias_manager instanceof CoreAliasManagerInterface) {
+        @trigger_error('The \\' . CoreAliasManagerInterface::class . ' interface is deprecated in drupal:8.8.0 and is removed from drupal:9.0.0. Instead, use \\' . AliasManagerInterface::class . '. See https://drupal.org/node/3092086', E_USER_DEPRECATED);
+      }
+      else {
+        $actual_class = isset($alias_manager) ? get_class($alias_manager) : 'NULL';
+        throw new \InvalidArgumentException('An instance of \\' . AliasManagerInterface::class . ' was expected \\' . $actual_class . ' provided.');
+      }
+    }
+
     $this->aliasManager = $alias_manager;

IMHO a slightly less complex version of this would be better. I'd just keep the outer condition and then do standard constructor BC message with fallback to \Drupal::service().

Something like:

@trigger_error('Calling SiteinformationForm::__construct() with CoreAliasManagerInterface::class instead of AliasManagerInterface::class is deprecated in drupal:8.8.0. The new service will be required in drupal:9.0.0. See https://www.drupal.org/node/3035273', E_USER_DEPRECATED);
$alias_manager = \Drupal::service('path_alias.manager');

The current message tries to address my unspecific, wishful-thinking comment about deprecating the interface, but this isn't the place for it and is actually missing the most important part.. *where* you need to make the change, the constructor of this specific class.

1. +++ b/core/lib/Drupal/Core/EventSubscriber/PathSubscriber.php
   @@ -12,10 +12,10 @@
     *
   - *   @deprecated in drupal:8.8.0 and is removed from drupal:9.0.0.
   - *   Use \Drupal\path_alias\EventSubscriber\PathAliasSubscriber.
   + * @deprecated in drupal:8.8.0 and is removed from drupal:9.0.0.
   + * Use \Drupal\path_alias\EventSubscriber\PathAliasSubscriber.
   

   Well, both are wrong, actually :) like @todo, @deprecated should be one space in and then the following lines 3.

   Wasn't sure if I should still set it to RTBC, but it is in there quite often, so maybe a bit much to clean up pre-commit?

2. +++ b/core/modules/path_alias/tests/src/Unit/DeprecatedClassesTest.php
   @@ -171,6 +171,33 @@ public function testAliasWhitelist() {
   +  /**
   +   * @covers \Drupal\system\Form\SiteInformationForm::__construct
   +   *
   +   * @expectedDeprecation Calling \Drupal\system\Form\SiteInformationForm::__construct with \Drupal\Core\Path\AliasManagerInterface instead of \Drupal\path_alias\AliasManagerInterface is deprecated in drupal:8.8.0. The new service will be required in drupal:9.0.0. See https://www.drupal.org/node/3092086
   +   */
   +  public function testDeprecatedSystemInformationFormConstructorParameters() {
   +    $this->assertDeprecatedConstructorParameter(SiteInformationForm::class);
   

   We usually don't bother with BC tests for constructors, entity manager conversion would probably still not be done if that would have been necessary. But we have it now, and since we're super late with this, it doesn't hurt to be extra careful :)

This is how I believe it should be, like other comments, it should only add linebreaks when the next word would be over 80 characters and mostly we finish it with .. instead. A little bit strange here as it's then always just the Use on the same line but so be it.

Edit: great, managed to upload it twice.

Lets say we've reviewed each others changes then, I just change those comments, so setting back to RTBC which it already was before by @jibran.

Yup, RTBC +1, the latest changes look good as well.

Had to resolve a minor merge conflict and a couple of coding style issues on commit to 9.0.x - which I've already pushed. But this doesn't apply to 8.9.x any more either, so needs a re-roll there.

Here you go.

Committed/pushed to 8.8.x and 8.8.x, thanks!

I think this broke 9.0.x HEAD -- no test runs back there against 9.0.x.
https://www.drupal.org/pift-ci-job/1474419

Reverted against 9.0.x (but left in 8.8.x and 8.9.x that had proper test runs).

Here is the reroll.

Here I have tried to re-roll the patch.

Took me a moment to understand this. These workspaces kernel tests were interacting with the path_aliases services without enabling the module. We didn't notice this in 8.x due to the BC in KernelTestBase that always enables it.

The interdiff should probably also be backported to 8.x branches for consistency?

(3094292-31-workspaces-path-alias.patch both the interdiff and the patch for D8)

Two more kernel test that need path_alias. Since this was hold back from 9.0.x, I'm actually wondering if we don't want to move forward here and just commit a patch that updates the code without BC? We just have to remove the BC layer & tests again anyway?

Here's a version for 9.0.x without the BC layer and legacy tests. The existing parts we can remove in #3092090: Remove legacy Path Alias subsystem. Thoughts? That's a bunch of classes that we don't have to touch again, at least not until we actually make the module optional.

Makes sense to skip adding the bc layer to 9.x only to take it away again and avoid relying on the bc layer in 8.x. Committed/pushed to 9.0.x/8.9.x/8.8.x, thanks!

This introduced test failures on PHP 7.0 so I've reverted it. I initially considered leaving it in 9.0 since it requires PHP 7.2 and is therefore not affected, but that would risk the 8.9 commit diverging from the 9.0 one which we don't want.

Per @larowlan:

on 7.0 you have to use __toString but that is deprecated in 7.1 where you have to use ::getName()
so perhaps we need version juggling :crying_cat_face:

> I initially considered leaving it in 9.0 since it requires PHP 7.2 and is therefore not affected, but that would risk the 8.9 commit diverging from the 9.0 one which we don't want.

The test that fails was not added to 9.0.x because it's only a legacy test for BC. So IMHO, for 9.0.x, #34 is still the patch that should be committed (again). Wouldn't have been necessary to revert there, but I see how this issue is becoming very confusing.

Also, for 8.8.x, only the follow-up commit for test consistency from #33 was reverted, which will not actually fix the test fails. As I said, this issue is a mess :-/

I actually don't have PHP 7.0, but my understanding is that this should work then. It does on 7.2. Note that the ? .. : check is necessary because we check with isset() below and (string) $type is an empty string.

Also, for consistency, again re-uploading the 9.0.x patch from #34.(which was actually named 33, I changed that but it's the same patch)

Sharp eyes, @Berdir. The two commits have the same title and were separated from each other in the log by other commits. I've reverted the earlier one now... 🤞

OK another problem here is that there were no PHP 7.0 environments tested on commit. I think maybe I set it up that way thinking that PHP 7.0 is EOL, but it meant that we didn't know for days that it was broken -- only when I started reading my work email on a Sunday night. 🙄I've changed that so the 7.0 environment is tested on commit. We should probably leave it that way until EOL.

Oops, xposted status.

Aww, I was hoping we wouldn't have to do that, testbot is broken anyway right now :)

But fine, for 8.x, we now need a combined patch of the two commits against 8.8.x + the interdiff from #46, will do that later today if nobody beats me to it.

On it.

Here we go.

@xjm is https://www.drupal.org/commitlog/commit/2/1a3aa31f31b6f191bfd896732a9135... a mistake?

As discussed in slack, the revert of the revert in 9.0.x was not a mistake, per #46, the 7.0 bug exists in the legacy test for the BC layer that we did not commit to 9.0.x.

For the same reason, "3094292-55-9.0.x.patch" in comment #55 is the direct port of the D8 patch in the same comment, but we should commit "3094292-34-no-bc.patch" from #46 to 9.0.x.

It's probably cleaner to just to a regular commit of that again instead of reverting the revert of the revert of the revert :p

I did verify that the 8.8.x patch does what it should by comparing it against the commit before the revert-dance started:

$ git diff f2407f9fc4
diff --git a/core/modules/content_moderation/tests/src/Kernel/ContentModerationStateTest.php b/core/modules/content_moderation/tests/src/Kernel/ContentModerationStateTest.php
index 3e17d0beda..80984331f2 100644
--- a/core/modules/content_moderation/tests/src/Kernel/ContentModerationStateTest.php
+++ b/core/modules/content_moderation/tests/src/Kernel/ContentModerationStateTest.php
@@ -47,7 +47,7 @@ class ContentModerationStateTest extends KernelTestBase {
     'content_translation',
     'text',
     'workflows',
-    'path_alias',
+    'path_alias'
   ];
 
   /**
diff --git a/core/modules/path_alias/tests/src/Unit/DeprecatedClassesTest.php b/core/modules/path_alias/tests/src/Unit/DeprecatedClassesTest.php
index 347e778190..1b54403039 100644
--- a/core/modules/path_alias/tests/src/Unit/DeprecatedClassesTest.php
+++ b/core/modules/path_alias/tests/src/Unit/DeprecatedClassesTest.php
@@ -221,7 +221,7 @@ public function assertDeprecatedConstructorParameter($tested_class_name) {
       }
       else {
         $type = $parameter->getType();
-        $class_name = $type ? $type->getName() : NULL;
+        $class_name = $type ? (string) $type : NULL;
       }
       $args[$name] = isset($class_name) && $class_name !== 'array' ? $this->prophesize($class_name)->reveal() : [];
     }
diff --git a/core/modules/system/src/Form/SiteInformationForm.php b/core/modules/system/src/Form/SiteInformationForm.php
index c4d95b53de..1bf6550bac 100644
--- a/core/modules/system/src/Form/SiteInformationForm.php
+++ b/core/modules/system/src/Form/SiteInformationForm.php
@@ -55,7 +55,7 @@ public function __construct(ConfigFactoryInterface $config_factory, $alias_manag
     parent::__construct($config_factory);
 
     if (!$alias_manager instanceof AliasManagerInterface) {
-      @trigger_error('Calling \\' . __METHOD__ . ' with \\' . CoreAliasManagerInterface::class . ' instead of \\' . AliasManagerInterface::class . ' is deprecated in drupal:8.8.0. The new service will be required in drupal:9.0.0. See https://www.drupal.org/node/3092086', E_USER_DEPRECATED);
+      @trigger_error('Calling \\' . __METHOD__  . ' with \\' . CoreAliasManagerInterface::class . ' instead of \\' . AliasManagerInterface::class . ' is deprecated in drupal:8.8.0. The new service will be required in drupal:9.0.0. See https://www.drupal.org/node/3092086', E_USER_DEPRECATED);
       $alias_manager = \Drupal::service('path_alias.manager');
     }
 
diff --git a/core/modules/system/src/Plugin/Condition/RequestPath.php b/core/modules/system/src/Plugin/Condition/RequestPath.php
index 38dc216ca1..dc53be1183 100644
--- a/core/modules/system/src/Plugin/Condition/RequestPath.php
+++ b/core/modules/system/src/Plugin/Condition/RequestPath.php
@@ -72,7 +72,7 @@ public function __construct($alias_manager, PathMatcherInterface $path_matcher,
     parent::__construct($configuration, $plugin_id, $plugin_definition);
 
     if (!$alias_manager instanceof AliasManagerInterface) {
-      @trigger_error('Calling \\' . __METHOD__ . ' with \\' . CoreAliasManagerInterface::class . ' instead of \\' . AliasManagerInterface::class . ' is deprecated in drupal:8.8.0. The new service will be required in drupal:9.0.0. See https://www.drupal.org/node/3092086', E_USER_DEPRECATED);
+      @trigger_error('Calling \\' . __METHOD__  . ' with \\' . CoreAliasManagerInterface::class . ' instead of \\' . AliasManagerInterface::class . ' is deprecated in drupal:8.8.0. The new service will be required in drupal:9.0.0. See https://www.drupal.org/node/3092086', E_USER_DEPRECATED);
       $alias_manager = \Drupal::service('path_alias.manager');
     }
 
diff --git a/core/modules/views/src/Plugin/views/argument_default/Raw.php b/core/modules/views/src/Plugin/views/argument_default/Raw.php
index 048e37b9f9..ad5abe1cea 100644
--- a/core/modules/views/src/Plugin/views/argument_default/Raw.php
+++ b/core/modules/views/src/Plugin/views/argument_default/Raw.php
@@ -54,7 +54,7 @@ public function __construct(array $configuration, $plugin_id, $plugin_definition
     parent::__construct($configuration, $plugin_id, $plugin_definition);
 
     if (!$alias_manager instanceof AliasManagerInterface) {
-      @trigger_error('Calling \\' . __METHOD__ . ' with \\' . CoreAliasManagerInterface::class . ' instead of \\' . AliasManagerInterface::class . ' is deprecated in drupal:8.8.0. The new service will be required in drupal:9.0.0. See https://www.drupal.org/node/3092086', E_USER_DEPRECATED);
+      @trigger_error('Calling \\' . __METHOD__  . ' with \\' . CoreAliasManagerInterface::class . ' instead of \\' . AliasManagerInterface::class . ' is deprecated in drupal:8.8.0. The new service will be required in drupal:9.0.0. See https://www.drupal.org/node/3092086', E_USER_DEPRECATED);
       $alias_manager = \Drupal::service('path_alias.manager');
     }
 

So the only differences are the actual PHP 7.0 fix, whitespace fixes in the deprecation messages (which is again for code that doesn't need to be in 9.0.x) and that missing trailing comma for the module list in that one test, that can maybe be fixed on commit?

Committed/pushed to 9.0.x/8.9.x/8.8.x, hopefully for the last time ;) Thanks everyone!

🥁