Prepare for the conversion of path aliases to entities

Reroll with the new patch.

This time with a new patch.

This should pass more tests.

+++ b/src/AliasStorageHelper.php
@@ -117,6 +117,7 @@ class AliasStorageHelper implements AliasStorageHelperInterface {
           case PathautoGeneratorInterface::UPDATE_ACTION_DELETE:
             // The delete actions should overwrite the existing alias.
             $path_alias->setOriginalId($existing_alias->id());
+            $path_alias->set('id', $existing_alias->id());

I'm not sure about this change.

1. +++ b/src/AliasStorageHelper.php
   @@ -110,39 +109,43 @@ class AliasStorageHelper implements AliasStorageHelperInterface {
    
              case PathautoGeneratorInterface::UPDATE_ACTION_LEAVE:
   -            // Create a new alias instead of overwriting the existing by leaving
   -            // $path['pid'] empty.
   +            // Create a new alias instead of overwriting the existing one.
   +            $path_alias->enforceIsNew(TRUE);
                break;
    
              case PathautoGeneratorInterface::UPDATE_ACTION_DELETE:
                // The delete actions should overwrite the existing alias.
   -            $path['pid'] = $existing_alias['pid'];
   +            $new_alias = $path_alias->getAlias();
   +            $path_alias = clone $existing_alias;
   +            $path_alias->pathauto = TRUE;
   +            $path_alias->original = $existing_alias;
   +            $path_alias->setAlias($new_alias);
                break;
   
   @@ -150,19 +153,7 @@ class AliasStorageHelper implements AliasStorageHelperInterface {
   +    return $this->pathAliasStorage->lookupBySystemPath($source, $language);
   

   This stuff is all a bit scary and I'm wondering how much of it is really still necessary.

   Wondering if we should rethink that save() API. I don't really care about API stability of this, we're breaking it anyway by expecting a path entity object now. See below.

2. +++ b/src/AliasStorageHelper.php
   @@ -196,60 +191,30 @@ class AliasStorageHelper implements AliasStorageHelperInterface {
      /**
       * {@inheritdoc}
       */
      public function countAll() {
   -    return $this->database->select('url_alias')
   

   also open to removing methods that are now official supported by the API.

3. +++ b/src/PathautoGenerator.php
   @@ -264,13 +265,13 @@ class PathautoGenerator implements PathautoGeneratorInterface {
        // Build the new path alias array and send it off to be created.
   -    $path = [
   -      'source' => $source,
   +    $path_alias = PathAlias::create([
   +      'path' => $source,
          'alias' => $alias,
   -      'language' => $langcode,
   -    ];
   +      'langcode' => $langcode,
   +    ]);
    
   -    $return = $this->aliasStorageHelper->save($path, $existing_alias, $op);
   +    $return  = $this->aliasStorageHelper->save($path_alias, $existing_alias, $op);
    
   

   Yes, as I suspected...

   Wouldn't it become much easier if we'd just decide here if we should update or duplicate? Either you update $existing_alias and save, or create a new and save.

4. +++ b/src/Plugin/pathauto/AliasType/EntityAliasTypeBase.php
   @@ -147,16 +147,16 @@ class EntityAliasTypeBase extends ContextAwarePluginBase implements AliasTypeInt
    
        $query = $this->database->select($entity_type->get('base_table'), 'base_table');
   -    $query->leftJoin('url_alias', 'ua', "CONCAT('" . $this->getSourcePrefix() . "' , base_table.$id_key) = ua.source");
   +    $query->leftJoin('path_alias', 'pa', "CONCAT('" . $this->getSourcePrefix() . "' , base_table.$id_key) = pa.path");
        $query->addField('base_table', $id_key, 'id');
    
        switch ($action) {
          case 'create':
   

   these batch queries are ugly, but there isn't really a way to abstract with these custom conditions, and looping over all would result in massively slowing it down, especially when updating e.g. just term aliases and you have 1M node aliases.

Nice to see that we can improve the code. Let's update IS with API changes and let's start a draft change record as well.

1. Yeah, that's a good idea.

What I was wondering about is whether we could make a patch that works with both 8.7 and 8.8, that would mean we would have to add these methods back, and we could possibly even have two different implementations for 8.7 and 8.8 that we could register dynamically. That should work quite well. The generator and some other parts might be uglier?

While more work, I think that might also be beneficial for the being able to get the core issue in and we could commit this early, making testing and updating easier?

#2336597: Convert path aliases to full featured entities is in so what is the plan here? Are we going to create a new 2.x branch?

Patches are not applying to latest dev version.

The patch in #16 was able to apply for me. However it causes breaking issues. The patch references an interface that doesn't exist and methods on the alias storage that don't exist. Those methods however exist in the path.alias_storage service. Adding that service and changing the method calls appears to help out. I noticed these errors when trying to save after editing an existing node.

I've included the interdiff but it doesn't appear accurate to me. Forgive me if there's a better way to make the interdiff.

Patch #21 applies nicely and it looks like it's working as expected.

Thanks for working on this, note that we're currently explicitly holding out until 8.8.0-alpha1 is released next week, there are still some more things going on that will hopefully get in until then.

It's too early to test 8.8 + pathauto right now, give us another week to finalize this. I can guarantee that I will release a compatible version in time, don't know yet if that will be a new 2.x branch or not, I'd rather not and would prefer to just mark our own storage helper as internal, I hope people didn't rely on that too much.

After applying patch #21:

TypeError: Argument 3 passed to Drupal\pathauto\AliasStorageHelper::__construct() must implement interface Drupal\Core\Entity\EntityTypeManagerInterface, instance of Drupal\Core\Database\Driver\mysql\Connection given, called in /DATA/home-sites/drupal_8/web/core/lib/Drupal/Component/DependencyInjection/Container.php on line 285 in Drupal\pathauto\AliasStorageHelper->__construct() (line 80 of /DATA/home-sites/drupal_8/web/modules/contrib/pathauto/src/AliasStorageHelper.php)

CR fixed this. Sorry.

#21 fixed the problem I had with core 8.8.x where I couldn't delete user accounts, or bulk update path aliases (I got PHP errors / database exceptions complaining that Table 'mydbname.url_alias' doesn't exist -- happy to offer more info if it's useful, but it seems like things are good now, as far as I can tell).

FWIW, I had some trouble applying the patch via composer -- but I'm not convinced it was a problem with the patch, I may have been in one of those composer twilight zones that happens from time to time -- and eventually it worked, I just had to do some hacky manual updates -- point is, I feel like it was specific to my situation, not the patch, but I'm sharing anyway "just in case" someone else notices it, too, or whatever.

Thanks so much, all!

Locale test is failing for last patch see https://www.drupal.org/pift-ci-job/1439407.

8.8.0-alpha1 is out, so this is un-blocked.

+++ b/src/AliasStorageHelper.php
@@ -53,26 +54,36 @@ class AliasStorageHelper implements AliasStorageHelperInterface {
    *   The config factory.
-   * @param \Drupal\Core\Entity\EntityTypeManagerInterface $entity_type_manager
-   *   The entity type manger.
+   * @param \Drupal\Core\Path\AliasStorageInterface $alias_storage
+   *   The alias storage.
    * @param \Drupal\Core\Database\Connection $database
    *   The database connection.
    * @param MessengerInterface $messenger
    *   The messenger.
    * @param \Drupal\Core\StringTranslation\TranslationInterface $string_translation
    *   The string translation service.
+   * @param \Drupal\Core\Entity\EntityTypeManagerInterface $entity_type_manager
+   *   The entity type manger.
    */
-  public function __construct(ConfigFactoryInterface $config_factory, EntityTypeManagerInterface $entity_type_manager, Connection $database, MessengerInterface $messenger, TranslationInterface $string_translation) {
+  public function __construct(ConfigFactoryInterface $config_factory, AliasStorageInterface $alias_storage, Connection $database, MessengerInterface $messenger, TranslationInterface $string_translation, EntityTypeManagerInterface $entity_type_manager = NULL) {
     $this->configFactory = $config_factory;
-    $this->pathAliasStorage = $entity_type_manager->getStorage('path_alias');
+    $this->aliasStorage = $alias_storage;
     $this->database = $database;
     $this->messenger = $messenger;
     $this->stringTranslation = $string_translation;
+    $this->entityTypeManager = $entity_type_manager ?: \Drupal::service('entity_type.manager');
   }
 
   /**
@@ -94,7 +105,7 @@ class AliasStorageHelper implements AliasStorageHelperInterface {

This seems a bit inconsistent/strange.

We move entity type manager argument to the end and replace it with alias storage? Why not add alias storage to the end?

Also we check entity type manager for optional but not alias storage.

Yeah, I mixed up the patch files there somehow. This looks fine I think, committed.