Move all the code related to path aliases to a new (required) "path_alias" module

Closed #3084874: Consider moving path aliases to a new path_alias module as a duplicate. @catch posted a few questions in the IS over there:

Do we move the aliases to path module (making it required), or to a new path alias module (which would also need to be required, at least to start)?

How do we deal with the change in provider?

Of course the latter question is relevant only if we're not able to make this happen in 8.8.

Minor fix

This takes care of the site information form.

Don't we need to make path module required? If so then all the assumptions that the system is available shouldn't need to change (obviously they should eventually but not to make tests pass).

Ideally we want a patch here that doesn't require any changes outside Drupal\Core and path module since that will prove this is a non-disruptive change.

Yes, I think we discussed making this required for D8 and optional in D9, right? So in the latter case we would still need some test changes, but that would be follow-up work, I think.

We need to make it required in 8.x not because it has to be technically, but so that there is no way for sites to accidentally delete all their path aliases in 8.x (or any other scenario which might result from uninstalling it). In 9.x or 10.x we can make it optional, but it would need to be explicitly added to install profiles and similar.

+++ b/core/modules/path/src/PathAliasInterface.php
@@ -1,6 +1,6 @@
diff --git a/core/lib/Drupal/Core/Path/PathAliasStorage.php b/core/modules/path/src/PathAliasStorage.php

diff --git a/core/lib/Drupal/Core/Path/PathAliasStorage.php b/core/modules/path/src/PathAliasStorage.php
similarity index 98%

similarity index 98%
rename from core/lib/Drupal/Core/Path/PathAliasStorage.php

rename from core/lib/Drupal/Core/Path/PathAliasStorage.php
rename to core/modules/path/src/PathAliasStorage.php

rename to core/modules/path/src/PathAliasStorage.php
index a9db234c45..65e630ffe1 100644

index a9db234c45..65e630ffe1 100644
--- a/core/lib/Drupal/Core/Path/PathAliasStorage.php

--- a/core/lib/Drupal/Core/Path/PathAliasStorage.php
+++ b/core/modules/path/src/PathAliasStorage.php

+++ b/core/modules/path/src/PathAliasStorage.php
+++ b/core/modules/path/src/PathAliasStorage.php
@@ -1,6 +1,6 @@

@@ -1,6 +1,6 @@
 <?php
 
-namespace Drupal\Core\Path;
+namespace Drupal\path;
 

Even if we keep the old service names (we could also have aliases that we deprecated I guess), we still need to have BC for the classes as well, probably by leaving the old classes there and adding a @deprected, for people we used/subclassed them somhow directly.

I think having them just be an empty OldName extends NewName would be problematic as we can't prevent them from being loaded before the module is enabled.

> @Berdir, how about using class_alias() instead of leaving duplicated code around? That should allow existing code to work just as before.

Defined inside path.module then? Would still fail if a module subclasses one of these classes, e.g. a custom replacement of one of these services through a ServiceProvider, we can't fix this before these are called. I think the safest option is to just leave them where they are now and add @deprecated.

Actually, isn't it the same with the services? Looks like there are no core modules that inject any of these path services directly in another service, only in on-demand create() methods? But I'm pretty sure trying to update a site with e.g. pathauto or redirect enabled will fail hard on a site that has an empty cache. So IMHO same here, we need to leave the old services in place, pointing to the deprecated classes.

FWIW, that was my main concern in the initial issue about moving this to a module. Moving code/services around isn't a big deal, sure, especially if we keep it as required, but BC is *hard* :)

Working a bit on this

After discussing with @Berdir and @catch, I'm trying an approach that should limit the amount of changes to the minimum. We still need some test adjustments, especially to kernel tests. To rule out trivial kernel test failures only caused by the missing path_alias schema this patch tweaks KernelTestBase to always install it, however the plan is remove that and fix all Kernel tests in the final version.

Unassigning for now if anyone wishes to take a look at the last failures, I will get back to it later

Back on this

Just a reroll for now

No luck so far

Another reroll after #2233595: Deprecate the custom path alias storage.

This should be better.

+++ b/core/core.services.yml
@@ -1754,3 +1749,32 @@ services:
+    arguments: [path_alias_whitelist, '@cache.bootstrap', '@lock', '@state', 'path_alias.repository']

geez...

This should have way less test failures.

More fixes

A few more fixes.

This should fix the last failures. Setting to needs review, but still needs work actually, I will summarize tomorrow.

Here's the reason why I think this still needs work: the current patch, to be able to raise deprecation errors when a legacy path alias service is retrieved, defines a new set of services, instead of aliasing them, since the ability to deprecate aliases was introduced only in Symfony 4.3.

However this turned out to be a bad choice, since this way we'd have two path alias subscriber instances and (most importantly) two path alias processor ones executed, which would mean that lookups are doubled, since the service state is not shared, which I wrongly assumed would be the case. This is not acceptable, I believe.

For this reason I think we should move back to aliases and rely on class-level deprecation messages to warn API consumers. Unfortunately this won't work for what I believe is the most common use case, that is just performing path alias lookups through the alias manager, because PathAliasServiceProvider will swap the legacy class with the new one, which will cause deprecation errors not to be raised.

Maybe a possible solution could be a hybrid one: duplicate only the alias manager service, deprecate the old one, don't swap its implementation so that both container-level and class-level deprecation errors are raised, and somehow ensure that both instances rely on the same internal state, maybe via static members or some shared reference.

The other services (path_subscriber, path_processor_alias, and path.alias_whitelist) could be considered "internal" from the API usage perspective, even if they are not formally marked as such, meaning we would alias them, which wouldn't trigger explicit deprecation messages when retrieving them.

I think this would provide the best compromise between performance and DX, given the current state. From 9.0.0 on, when needing to deprecate services, we will be able to rely on deprecated aliases and this process will be way easier and reliable.

The other services (path_subscriber, path_processor_alias, and path.alias_whitelist

So path_subscriber and path_processor_alias are event subscribers/tagged services respectively, I think these can be straightforwardly removed from a minor release (as we might remove a hook implementation). We'd leave the actual code in core and deprecated, but remove the service definition.

path.alias_whitelist is tagged as needs destruction but is injected into other services. Would "move back to aliases and rely on class-level deprecation messages to warn API consumers" work for that?

somehow ensure that both instances rely on the same internal state

Depending on what state needs to be shared, MemoryCache may help there.

This implements #51. Unfortunately aliasing the whitelist means we don't get deprecation messages for it, because the actual implementation class is not deprecated.

Updated IS, added CR and follow-up to remove the legacy code.

Also, marked as requiring CR updates to adjust the alias repository CR once this is committed.

This updates the previous patch with all the missing CR and @todo references and performs a final PHP doc cleanup. We should be ready for review now.

This provides also a "review" patch that doesn't include the fixture changes that are bloating the actual patch.

Maybe it makes sense to split path_alias and path_alias_ui

This adds explicit test coverage for the DB update changes.

@andypost, #55:

There is no UI code involved here: the UI is entirely provided by the path module. I intentionally did not update any module name/description and marked the path_alias module hidden to avoid introducing any user-facing change.

1. +++ b/core/core.services.yml
   @@ -1222,11 +1221,6 @@ services:
        tags:
          - { name: event_subscriber }
   -  path_subscriber:
   -    class: Drupal\Core\EventSubscriber\PathSubscriber
   -    tags:
   -      - { name: event_subscriber }
   -    arguments: ['@path.alias_manager', '@path.current']
      route_access_response_subscriber:
        class: Drupal\Core\EventSubscriber\RouteAccessResponseSubscriber
        tags:
   @@ -1357,12 +1351,6 @@ services:
   
   @@ -1357,12 +1351,6 @@ services:
        arguments: ['@current_route_match']
        tags:
          - { name: route_processor_outbound, priority: 200 }
   -  path_processor_alias:
   -    class: Drupal\Core\PathProcessor\PathProcessorAlias
   -    tags:
   -      - { name: path_processor_inbound, priority: 100 }
   -      - { name: path_processor_outbound, priority: 300 }
   -    arguments: ['@path.alias_manager']
      route_processor_csrf:
   

   I think it's fine to remove these outright, because nothing should interact with them directly.

   The one thing that might happen is that someone has code altering the service definitions, which is equivalent to doing hook_module_implements_alter(). If this is the case, then it's reasonable for them to need to update their code for a minor. Nothing should be using the services directly in the same way you wouldn't call a hook implementation directly.

2. +++ b/core/core.services.yml
   @@ -1754,3 +1742,32 @@ services:
   +
   +  # Path Alias services, defined here to be available even when "path_alias" is
   +  # not enabled yet. These will replace the legacy core services, which are now
   +  # deprecated and will be removed in Drupal 9.
   +  # See https://www.drupal.org/node/3092086
   

   Do these also need a @todo to remove in Drupal 9 - they should be OK to remove in the first release where we can guarantee path_alias module is enabled.

3. +++ b/core/lib/Drupal/Core/Path/AliasManager.php
   @@ -115,12 +121,37 @@ class AliasManager implements AliasManagerInterface, CacheDecoratorInterface {
   +
   +      // Despite being two different services, hence two different class
   +      // instances, both the new and the legacy alias managers need to share the
   +      // same internal state to keep the path/alias lookup optimizations
   +      // working.
   +      try {
   +        $alias_manager = \Drupal::service('path_alias.manager');
   +        if ($alias_manager instanceof $new_class) {
   +          $synced_properties = ['cacheKey', 'langcodePreloaded', 'lookupMap', 'noAlias', 'noPath', 'preloadedPathLookups'];
   +          foreach ($synced_properties as $property) {
   +            $this->{$property} = &$alias_manager->{$property};
   +          }
   +        }
   +      }
   

   Oof. But can't think of another way to do it, and at least it's temporary.

4. +++ b/core/modules/path/path.module
   @@ -48,14 +48,16 @@ function path_help($route_name, RouteMatchInterface $route_match) {
     */
    function path_entity_type_alter(array &$entity_types) {
      /** @var $entity_types \Drupal\Core\Entity\EntityTypeInterface[] */
   -  $entity_types['path_alias']->setFormClass('default', PathAliasForm::class);
   -  $entity_types['path_alias']->setFormClass('delete', ContentEntityDeleteForm::class);
   -  $entity_types['path_alias']->setHandlerClass('route_provider', ['html' => AdminHtmlRouteProvider::class]);
   -  $entity_types['path_alias']->setListBuilderClass(PathAliasListBuilder::class);
   -  $entity_types['path_alias']->setLinkTemplate('collection', '/admin/config/search/path');
   -  $entity_types['path_alias']->setLinkTemplate('add-form', '/admin/config/search/path/add');
   -  $entity_types['path_alias']->setLinkTemplate('edit-form', '/admin/config/search/path/edit/{path_alias}');
   -  $entity_types['path_alias']->setLinkTemplate('delete-form', '/admin/config/search/path/delete/{path_alias}');
   +  if (isset($entity_types['path_alias'])) {
   +    $entity_types['path_alias']->setFormClass('default', PathAliasForm::class);
   

   Path module should have a dependency on path_alias module, so is this only needed to avoid errors during the update? If so we should add a @todo to remove the conditional.

5. +++ b/core/modules/path_alias/src/PathAliasServiceProvider.php
   @@ -0,0 +1,76 @@
   +
   +/**
   + * Path Alias service provider.
   + *
   + * Updates core path alias service definitions to use the new classes provided
   + * by "path_alias" and marks the old ones as deprecated. The "path_alias.*"
   + * services defined in "core.services.yml" will bridge the gap.
   + *
   + * @see https://www.drupal.org/node/3092086
   + */
   

   Probably also needs a @todo to remove in Drupal 9.

Added to the release notes snippet to note the lack of upgrade path from 8.8.0-alpha1.

+++ b/core/MAINTAINERS.txt
@@ -318,6 +318,9 @@ Page Cache
+Path Alias
+- ?
+

I'm not sure about adding a new module with no maintainer?

Who is responsible for the subsystem with the current architecture? I mean @catch's name is on "Path" so maybe this is effectively the same state as HEAD, just refactored?

I'm still technically the path maintainer, but although I do try to review any major path system patches I don't actively maintain it obviously.

So the ? is more honest, but putting my name there wouldn't be less honest than the situation in HEAD.

I think the update helper script is a lot more reliable in case there are sites that want to update from the alpha - it saves trying to account for two completely different database states in the 'official' update path, which adds a lot of complexity, but there's still a way for people with broken alpha installs to repair their site if they need to.

1. +++ b/core/modules/path_alias/tests/modules/path_alias_deprecated_test/path_alias_deprecated_test.module
   @@ -8,20 +8,20 @@
     */
   -function path_deprecated_test_path_insert($path) {
   +function path_alias_deprecated_test_path_insert($path) {
      // Nothing to do here.
    }
    
    /**
     * Implements hook_path_update().
   

   A bit confused about the rename of the test module, that was just there to test the old hooks, it's path hooks that are deprecated, not path_alias hooks? I see that it is moved from system/tests to pathauto/tests, but that too seems a bit strange, why should path_alias be responsible for testing what has been deprecated in \Drupal\Core\Path?

2. +++ b/core/tests/Drupal/KernelTests/Core/Routing/RouteProviderTest.php
   @@ -38,7 +38,7 @@ class RouteProviderTest extends KernelTestBase {
      /**
       * Modules to enable.
       */
   -  public static $modules = ['url_alter_test', 'system', 'language'];
   +  public static $modules = ['url_alter_test', 'system', 'language', 'path_alias'];
    
      /**
   

   This change is going to break contrib kernel tests that interact with the alias storage again. A bit annoying for the projects that already updated their tests for D8.8 :-/

Thanks for the reviews, the attached patch should address them. Including an updated review patch.

@amateescu, #58:

1: That was a trick to prevent PHPUnit from complaining about deprecated classes backing non-deprecated services. Unfortunately I was not able to find any other workaround besides "breaking" its parsing logic.

Removed the core compatibility key from the new test module and rerolled also for D9, since there were conflicts.

Edit: the interdiff is reversed, sorry :)

Please, disregard #66, patches were quite garbled. Here's a pair of new ones:

• path-alias_module-3084983-67.d8.patch is a straight reroll of #64 on 8.9.x.
• path-alias_module-3084983-67.d9.patch is a straight reroll of #64 on 9.0.x with the diff in path-alias_module-3084983-67.d8-d9-diff.txt applied.

OK I'm not finding anything much to complain about in the patch - the backwards compatibility layers are not pretty, but they're temporary.

The main question is whether this can still get into the 8.8.x alpha.

The thing that makes this tricky is:

 function system_update_8803() {
+  // Enable the Path module if needed.
+  if (!\Drupal::moduleHandler()->moduleExists('path_alias')) {
+    \Drupal::service('module_installer')->install(['path_alias'], FALSE);
+  }
+
   $entity_definition_update_manager = \Drupal::entityDefinitionUpdateManager();
   if (!$entity_definition_update_manager->getEntityType('path_alias')) {

i.e. we have to enable the path_alias module prior to adding the path alias entity type, since both will happen in the same update when updating from Drupal 8.7.x, and the module has to be enabled for its entity type to be installed.

A site on 8.8.0-alpha1 will already have the path alias entity type installed, so the upgrade path from the alpha would be completely different: enabling the module then re-assigning the provider for the already-existing entity type.

This is what makes it difficult to provide an update from both 8.7 and 8.8 alpha - that there would be two completely different code paths to support, and it would add risk to the 8.7-8.8 upgrade path (the majority of updates) trying to support both.

We're pretty sure we can provide a one-off script to update from alpha to beta (enabling the module and switching the entity type provider) - which is a much more known quantity if you know you're dealing with that situation than dynamically switching the upgrade path for everyone, and the script would only be necessary for a handful of sites at most.

The further question is what happens if this doesn't land in 8.8.0-alpha1. We won't be able to do any new 9.x deprecations in 8.9.x, but we could potentially commit it (to either 8.9.x or 9.1.x) with deprecations for Drupal 10. One advantage of a Drupal 9 commit is that Symfony 4 supports deprecated aliases, which would remove some of the bc layer ugliness here, although obviously removing the deprecations altogether in Drupal 9 would be nice too.

For a post-8.8 commit the upgrade path looks like getting the module enabled and switching the provider - i.e. the same thing that would need to happen in an alpha-beta update script more or less.

One last thing, if this doesn't land in Drupal 8.8, there is one thing we can do to make things easier in Drupal 9.

The whole point of moving this code to a module, is to make the entire path alias subsystem optional in Drupal 9. However it's easier to make the module optional if we know that it's already required and enabled on every site. If we add the new module in 9.0.x then make it optional in 9.3.x, a site could update from 8.9.x to 9.3.x which might be tricky.

Therefore, as a fallback in 8.8.x, we could just add a path_alias stub module with .info.yml only, as a new required (and potentially hidden) module, then future updates are just moving code as opposed to trying to enable the new module at the same time.

Tagging with needs release manager review so xjm gets the final decision here - while I didn't work on the patch I'm a bit biased towards path alias system clean-up.

So all things considered I guess I do agree it's preferable to cluster these changes in the same minor release as #2336597: Convert path aliases to full featured entities and friends. That way we're only altering the API and data model once, rather than twice, and very few sites are probably using this.

That said, could we have a new update hook that's conditional on whether the module's enabled or not? So we retain the conditional change in system_update_8803() that enables the module, but then also add a separate update hook in 8805 that does the same thing (plus updates the entity definition's provider to be path_alias.module as @catch points out).

+++ b/core/modules/system/system.install
@@ -2555,3 +2555,39 @@ function system_update_8804(&$sandbox = NULL) {
+  // system_update_8803() ran as part of Drupal 8.8.0-alpha1, in which case we

Do we want to add a dedicated DB dump of 8.8.0-alpha1 to test this?

I don't think we should add test coverage for 8.8.0-alpha1, the upgrade path is 'best effort' since alphas aren't supported at all for production sites, similar to how we don't add test coverage for constructor bc layers. #70 looks good.

While working on the update path @amateescu found this issue: #3092855: Installing a module in an update function should not result in automatically installing entity type/field definitions.

@amateescu, @catch, and I briefly discussed it and decided that this should be postponed on it, otherwise it would be introducing a potential update path issue.

Moving back to RTBC per #3092855-17: Installing a module in an update function should not result in automatically installing entity type/field definitions.

(for reals now)

@amateescu pointed out in Slack that we can remove the previous definition installations from system_update_8803(): sites having already run it won’t care and sites not having run it yet won’t need them.

This also removes the core keys after #3072702: Core extensions should not need to specify the new core_version_requirement in *.info.yml files so that 9.0.x can be installed landed, which makes the D9 version of the patch identical to D8, aside from context differences.

Is it worth repeating the manual testing from #65 for #76?

That said, this seems like a good approach for the upgrade path and OK to backport to 8.8.x still so that it's in the same minor as the other changes.

We probably need to mention this in the 9.0.0 release notes as well just as a reminder that sites or modules will need to add a dependency on or check for the new module for stuff to continue working as expected.

#76 definitely won't make any difference for sites on the alpha, it would only affect sites upgrading from 8.7 but that's covered by the upgrade path tests.

Committed/pushed to 9.0.x, 8.9.x and 8.8.x

Probably the patch that actually makes path module optional in 9.x will be the one that ends up in the release notes, but let's leave this tagged anyway.

Tricky issue but very glad we were able to get it done, enables a lot more clean-up in 9.x

Published the change record: https://www.drupal.org/node/3092086.

This breaks all contrib Kernel tests that have anything to do with path aliases, since they now have to install path_alias separately.
See, for example #3089688: Make the build pass on Drupal 8.8
That's cool, the drop is always moving and all. ;)

However, the way we handle \Drupal::VERSION during alpha/beta makes it even harder for contrib to continue to support 8.7.x and 8.8.x-dev.
@see #3093130: Between alpha1 and X.Y.0 official \Drupal::VERSION should not be "8.8.0-dev" since it doesn't work for Semver::satisfies()

Any objections to adding some of that wisdom to the (now published) CR so other contrib maintainers can keep their Kernel tests working on the currently supported versions of core?

Thanks,
-Derek

I added https://www.drupal.org/node/3092086/revisions/view/11636531/11636895 for now. Feel free to make further edits as desired.

Thanks,
-Derek

@dww:

Very useful addition, Derek, thank you!

@plach re: #86 you're welcome!

However, I'm happy to report that I just ripped it back out of the CR, since a much better solution is now in 8.8.x:

#3093257: Install path_alias by default in kernel tests to minimize the impact on contrib tests supporting 8.7.x

:) Yay!

Cheers,
-Derek

This is no longer incompatible with alpha1, so updating the release note.

This has broken subpathauto module see #3093377: Fix the failing HEAD.

Also pathauto: #3093401: Compatibility with Drupal 8.8.0-beta1, although we'd have almost managed to make it compatible if we wouldn't have hardcoded the entity class name ;)

One thing I noticed there is that the BC layer is quite confusing, as two people including @jibran tried to "fix" my patch which uses the non-deprecated interface.

Makes me wonder if we could somehow invert that definition and define the real services in path_alias.services.yml and add a fallback in \Drupal\Core\CoreServiceProvider::alter() if they aren't there?

Also, the reason this actually broke subpathauto is that it uses the path_processor_alias service directly, for which we didn't provide BC. It would be trivial for us to keep that service in place with a deprecation warning so that subpathauto doesn't need to either require 8.8 or add workarounds per my comment over there?

When we change the core/composer.json, we should also update the composer.lock.

See also #3089007: There is no information in the documentation about creating patches with the changes in composer.json and composer.lock files.

> Also, the reason this actually broke subpathauto is that it uses the path_processor_alias service directly, for which we didn't provide BC. It would be trivial for us to keep that service in place with a deprecation warning so that subpathauto doesn't need to either require 8.8 or add workarounds per my comment over there?

Ah, might not be so trivial after all. It couldn't be a tagged service, or it would be called twice. So we could maybe keep the service without the tags, in case someone had injected it. Still not quite sure why subpathauto even did that, didn't check the code.

#92 looks good, thanks!

Here are the 8.8 and 9.0 versions of #92.

Committed/pushed to 9.0.x, 8.9.x, 8.8.x, thanks!

Updated related CR:

https://www.drupal.org/node/3013865/revisions/view/11630585/11640815

The change to `system_update_8803()` is problematic. We should not enable modules during `hook_install()` since the database structure is in a broken state. This should be done in a post_update hook.

There is an issue filed for Organic Groups reporting that `system_update_8803()` breaks (ref. https://github.com/Gizra/og/issues/572). In this case the problem is that by installing a module the entire router is being rebuilt. This seems a very risky operation when the database is not in a known good state.

A post update would be too late for this I think, as without the module, the whole site is going to be in a fragile state and alias lookups would fail.