Use migration lookup on text fields

I was able to apply the patch(s) and replace entity ID based links in HTML body fields with relative ease. Thanks for working on this added feature.

The plugin in this patch extends the one added in #2958285: Allow replacing based on a xpath expression, so I am marking this issue postponed. If that issue is accepted in its current form, then we will also need to update this patch to be compatible with the new base class.

I have updated the plugin to use the base class we added in #2958285: Allow replacing based on a xpath expression, and I added a test class.

I think we should have a single change record for all the DOM-related process plugins. We might also want to update the project page.

I have not done any manual testing, but the automated tests are passing locally.

On #3042539: Apply styles configured for CKEditor, I asked about the two different approaches to creating mock objects. I think the getMock() one is deprecated, so here I continue to use prophesize() as I did on that issue.

I see that there is now an 8.x-5.x branch, and that is the only one that can be used for automated tests, so I have updated the issue metadata to refer to that branch. I generated the patch by comparing to the 8.x-4.x branch, but the patch applies cleanly to either branch.

I think the issue title is unclear.

While I am at it, I will update the issue description: after the previous comment, we do not have to list tests in the "Remaining tasks" section.

This is sooooooo cooooooooolllllllll............. I don't have a need for it at the moment, I just wanted to say, sooooooo coooooooooooollllll.

Question: If there's a single change record for all the DOM-related process plugins, would that mean they each have to wait to be rolled into the module til they're all done, or?

Really great work here. Just a few small comments.

1. +++ b/src/Plugin/migrate/process/DomMigrationLookupStrReplace.php
   @@ -0,0 +1,224 @@
   + *   parenthesized subpattern which will be used as the id passed to
   ...
   +   * Lookup the migration mapped id on one migration.
   ...
   +   *   The found mapped id, or NULL if not found on the provided migration.
   

   Nit: s/id/ID/

2. +++ b/src/Plugin/migrate/process/DomMigrationLookupStrReplace.php
   @@ -0,0 +1,224 @@
   + *       plugin: dom_migration_lookup_str_replace
   

   Naming... does dom_migration_lookup strike your fancy? I'm trying to think of something less wordy/long for the name of the plugin.

3. +++ b/src/Plugin/migrate/process/DomMigrationLookupStrReplace.php
   @@ -0,0 +1,224 @@
   +    $plugin_configuration = [
   +      'migration' => $migration_name,
   +      'no_stub' => TRUE,
   

   Could we make no_stub a configurable option to this plugin with a default of TRUE?

@alisonjo2786:

Thanks. It is also useful!

@heddn:

Thanks for the review!

1. Done. I even found another:
   

   -   *   The id to search with migration_lookup process plugin.
   +   *   The ID to search with migration_lookup process plugin.
   

2. I also prefer the shorter version. Less is more. I checked with @marvil07. He still prefers the longer version as "more descriptive", but he is willing to go along with the shorter name. I updated the class names (plugin and test) to match, and of course also the file names.
3. Done. At first, I forgot to add the default value, but the unit test reminded me to do that. I thought about adding a test for no_stub: false, but that seems hard to do properly.

Normally I use the interdiff utility because there is more room for error when using git. In this case, the utility generated an interdiff with both files removed and then new ones added (because I renamed the files) so I used git.

I still owe you a change record.

I drafted a change record: Add process plugins for HTML processing using DOMDocument objects. It describes the process plugins added in this issue and the other children of #2958642: DOM manipulation on process plugins.

I am not sure what the next planned release is for Migrate Plus: 8.x-4.3 or 8.x-5.0.

It is out of scope for the current issue, but while writing the CR, I noticed an inconsistency. The dom_migration_lookup plugin introduced in this issue and the dom_str_replace plugin added in the 8.x-4.2 release use expression as the parameter for the XPath expression. The dom_apply_styles plugin from #3042539: Apply styles configured for CKEditor uses xpath for the same thing.

Should we make these consistent? Personally, I prefer xpath. Since 8.x-4.2 already contains a plugin that uses expression, we would have to keep that as an alias, but deprecate it. Alternatively, we could modify the dom_apply_styles plugin to be consistent with the others. Since that is only in the dev release, I think we can change it without supporting the original syntax.

@benjifisher Oh that's a great catch -- I agree about using xpath for the config key, expression seems more generic (like, even in this specific context, you've got "regular expression" right there). But, good point that we'd have to keep "expression" at least as an alias... Is that a hassle, or no big deal, or?

I strongly prefer xpath, but, I'd rather have the consistency and settle for expression than not have the consistency.

(Happy to help implement, when there's a decision!)

5.0 is the next planned release. 4.x has BC issues with 9.x and doesn't have any further planned development. See #3052510: Entity manager deprecation fixes. Since 5.x doesn't have a stable release yet, we could just make the fixes for xpath vs expression and mention in release notes / change record.

Leaving in NR to ponder these things. But otherwise, this is looking really good.

We discussed this in the #3067311: [meeting] Migrate Meeting 2019-07-11.

• @heddn suggested that, since we are targeting the 8.x-5.0 release, we are allowed to break BC (changing the parameter name from "expression" to "xpath" for the dom_str_replace plugin).
• @marvil07 prefers to keep "expression" for consistency with the DOMXPath class.

I prefer "xpath", since "expression" seems too generic in this context, and @alisonjo2786 (#13 on this issue) agrees.

Does anyone else want to vote on whether to call the parameter "expression" or "xpath"? (I will also solicit opinions on the #migration Slack channel.)

Discussed on #migration slack channel -- came to a decision to change config key name from "expression" to "xpath" -- for this new plugin (dom_migration_lookup), and the plugin it extends (dom_str_replace).

As such -- see attached!

Since 5.x doesn't have a stable release yet, we could just make the fixes for xpath vs expression and mention in release notes / change record.

I made an initial update to the Change record -- is there a typical way to highlight a change within the changes like this? I'll work on something, but if anyone has an example, pls feel free to update it again / change what I put / point me to an example / etc.

+++ b/src/Plugin/migrate/process/DomMigrationLookup.php
@@ -19,7 +19,7 @@
+ * - xpath: XPath query expression that will produce the \DOMNodeList to
  *   walk.

+++ b/src/Plugin/migrate/process/DomStrReplace.php
@@ -16,7 +16,7 @@ use Drupal\migrate\Row;
+ * - xpath: XPath query expression that will produce the \DOMNodeList to
  *   walk.

Nit: same line? Will fix on commit.

Thanks to everyones contributions here. I'll tag a release with this too.

I moved dom_migrate_lookup to the list of available process plugins provided by Migrate Plus on the documentation page: https://www.drupal.org/docs/8/api/migrate-api/migrate-process-plugins/li...

@idebr: Thanks!