Convert path alias migrations to use entity:path_alias destination

This needs to be done before #2233595: Deprecate the custom path alias storage, so here's a patch that a proper destination for path_alias entities.

The combined patch includes the combined one from #2336597-88: Convert path aliases to full featured entities.

Rerolled the combined patch, the "for review" one is the same.

+++ b/core/modules/path/src/Plugin/migrate/destination/UrlAlias.php
@@ -2,100 +2,25 @@
+@trigger_error('UrlAlias is deprecated in Drupal 8.7.x and will be removed before Drupal 9.0.x. Use \Drupal\Core\Path\Plugin\migrate\destination\EntityPathAlias instead.', E_USER_DEPRECATED);

Needs CR and link to it in this trigger error.

There we go :)

This tag might be helpful some day.

Maybe this is jumping the gun a little, but I think we're good to go here now.

Thanks @heddn! Let's re-RTBC it when #2336597: Convert path aliases to full featured entities gets in. :)

Rerolled on top of the latest patch from #2336597: Convert path aliases to full featured entities and updated the deprecation messages.

#2336597: Convert path aliases to full featured entities is in.

The patch here hasn't changed since #5, so putting it straight to RTBC per #8 :)

The destination plugin is only modifying values on the row so shouldn't we be doing that in a process plugin?

Agreed, no need for a destination. Let's do this in a process plugin. Doing it in a process plugin is the right thing, but it does lead to BC considerations. Let's see what it looks like first though as a process plugin, then we can work out a BC solution.

This may still have failures and need some cleanup, but I wanted testbot to chew on it while I run to a meeting.

Passes tests and no coder issues, I think this is actually pretty close.

This should preserve BC as much as possible.
If you are using the old migration with the new codebase, we've converted the existing UrlAlias destination to write to the new entity storage, but deprecated it.

Running the new migrations on the new codebase uses the new process plugins and the standard entity destination.

I think we may need an explicit test of the legacy destination. I think we lost coverage there when we changed the destination in the migrations.

Setting to NW for that, I'll try to write one if I have time.

Legacy tests and BC fixes for the deprecated destination. Okay, I'm happy with this now.

Actually, let's go this route, it's cleaner.

Haven't read the whole patch but these caught my attention.

1. +++ b/core/modules/migrate/src/Plugin/migrate/process/NullCoalesce.php
   @@ -0,0 +1,44 @@
   + * Returns the first non-null value in an array.
   ...
   + *   id = "null_coalesce"
   

   Since this is not the same as the php null coalesce I don't think it should use the same name. It would be better to avoid confusion. There is an existing process plugin that works with an array, array_build, using that as precedent this one could be array_null_coalesce.

2. +++ b/core/modules/migrate/src/Plugin/migrate/process/NullCoalesce.php
   @@ -0,0 +1,44 @@
   +      throw new MigrateException("The null_coalesce plugin expects the source value to be an array.");
   

   The messages thrown in existing process plugin use 'should'. So this could be "The input value should be an array." as is used in the extract plugin. And with the plugin id it would be ,'The input value to the null_coalesce plugin should be an array.'. Or I am being too pedantic.

3. +++ b/core/modules/migrate/src/Plugin/migrate/process/NullCoalesce.php
   @@ -0,0 +1,44 @@
   +    if (isset($this->configuration['default_value'])) {
   +      return $this->configuration['default_value'];
   +    }
   +    return NULL;
   

   Why not use ?? here?

4. +++ b/core/modules/migrate/tests/src/Unit/process/NullCoalesceTest.php
   @@ -0,0 +1,52 @@
   +class NullCoalesceTest extends MigrateProcessTestCase {
   

   Needs more tests. Specifically, with inputs other than NULL.

5. +++ b/core/modules/path/migrations/d6_url_alias.yml
   @@ -38,5 +36,19 @@ process:
   +      plugin: null_coalesce
   +      source:
   +        - '@node_translation/1'
   +        - language
   +    -
   +      plugin: default_value
   

   Use the null_coalesce has a default configuration value instead of another plugin.

And set to NW

1. The intention here is to mirror the php Null Coalesce operator. I've updated the documentation to make that more clear and show how to use the plugin as such. I think the plugin name is appropriate.
2. I changed the wording on the exception.
3. I had a conversation regarding ?? and scalar type hints with a release manager a few days ago, and they requested that we not use either until 8.7 is EOL next June in order to help ease the upcoming burden of maintaining 4 separate branches.
4. I added a few more test cases via a data provider
5. The default_value for the null_coalesce would only apply if all inputs were NULL, but in this case, in the D6 migration the language would be an empty string. So we use the default value plugin to catch the empty string and replace it with 'und' . This reminds me, I'm doing this in lieu of using the d6_url_alias_language, which was doing the same thing. There should be a follow-up to deprecate that plugin.

Thanks for the fixes and explanations, mikelutz. I still disagree with the name but I am thinking about it.

Changed the title to emphasize the change in destination and a few more items.

1. +++ b/core/modules/path/src/Plugin/migrate/process/PathSetTranslated.php
   @@ -0,0 +1,38 @@
   + * A process Plugin to update the path of a translated node.
   

   Should Plugin be capitalized? I am not sure.

   This documentation needs usage examples. I'm ok with that being done in a followup.

2. +++ b/core/modules/path/src/Plugin/migrate/process/PathSetTranslated.php
   @@ -0,0 +1,38 @@
   +    if (preg_match('/^\/node\/\d+$/', $value[0]) && isset($value[1]) && is_array($value[1])) {
   

   Looks like this need to check that $value is an array?

Adding migrate tag

I added an example, fixed the doc, and added some additional error checking.

I wasn't as worried about documenting the Path specific plugin as it's pretty much for this specific migration, not a general use plugin. At some point in the drupal 9 cycle, I think I'd like to refactor and these migrations around the new migration lookup service, but that's not on my radar right now.

This looks great to me! Thanks a lot @mikelutz and @quietone for working on it :)

+++ b/core/modules/path/src/Plugin/migrate/process/PathSetTranslated.php
@@ -0,0 +1,75 @@
+    // @todo Use ?? operator below once Drupal 8.7 is EOL.

This patch is only for 8.8

Once I wrote the following null_coalescing plugin for a client project.

  public function transform($value, MigrateExecutableInterface $migrate_executable, Row $row, $destination_property) {
    if (!is_array($value)) {
      throw new MigrateException(sprintf('NullCoalescing process failed for destination property (%s): input is not an array.', $destination_property));
    }
    $new_value = NULL;
    foreach ($value as $i => $item) {
      if (!is_null($item) && !is_scalar($item)) {
        throw new MigrateException(sprintf('NullCoalescing process failed for destination property (%s): index (%s) in the source value is not a scalar.', $destination_property, $i));
      }
      $new_value = $new_value ?? $item;
    }
    return $new_value;
  }

It didn't have a default value support which we do have in this patch and it is a good thinking. Do we need to update https://www.drupal.org/docs/8/api/migrate-api/migrate-process-plugins/li... after the issue is committed?

This patch is only for 8.8

I just had a conversation with one of the Release Managers a few days ago, and was told not to use ?? or scalar typehints anyway, even in new files that will not be backported to 8.7. I'm just doing what I was told. :-)

I don't think we should get hung up on that new PHP syntax in this issue, because it's really important for the plan in #3007661: Modernize the path alias system.

This looks good to me, but is not my area of expertise, so I'd be happy to see an explicit +1 from a migrate subsystem maintainer

Yes this looks fine to me. I still have a niggle about the process plugin name, null_coalesce, because it operate on an array. But I don't think the process plugins that share a name with a php function do exactly the same as the php function. In that sense we are not breaking any precedent. Any as heddn says, naming things is hard. Let's get this in.

Found one minor thing that I hope can be fixed on commit. I can't think of a core migration that has a blank line.

+++ b/core/modules/path/migrations/d6_url_alias.yml
@@ -18,9 +21,7 @@ process:
+

delete blank line.

+1 for RTBC

Oh, and jibran is correct in #30 we need to update the list of process plugins, https://www.drupal.org/docs/8/api/migrate-api/migrate-process-plugins/li....

Committed 445132d and pushed to 9.0.x. Thanks!

c/p to 8.8 and 8.9

Can we have those change record updates and followups as per the tags, as well as the update to the process plugin list as per #30

thanks!

I'll update the process plugin list now as per #30.

Updated the CR (https://www.drupal.org/node/3013865) and opened the followup to deprecate the d6_url_alias_language process plugin: #3087332: Deprecate the 'd6_url_alias_language' migration process plugin

Follow-up created to refine the migrations now that they have an entity as a destination, and entities can now optionally be validate during migration: #3098985: Follow-up for #3009014: ensure migrations with PathAlias destinations run *after* locatable content entity type destinations.

I know this issue is long closed, but I agree with @quietone that null_coalesce is not a good name for the process plugin introduced here. In addition to being different from the php operator, it's just a cryptic name. I look at the name and I have no idea what it does.

Also it's relatively difficult to spell!

I, like @jibran, have written my own version of this plugin before and I call it first_things_first. That's fun and catchy. A name like first_value or first_not_null would be a lot clearer and easier to spell.

I think null_coalesce needs to get the same treatment that iterator got.

https://www.drupal.org/node/2880427

P.S It's a great addition to core nonetheless. Thanks for the work on this one. I wish there had been a change record that called this addition out on its own.