Allow manipulating html from process plugins

Nice! I've got a migration doing all sorts of stuff to a body field. It currently does a callback process to do all the magic directly in PHP in my custom migration module. It's conceivable it could be made to work with this approach, instead. Given my background, I tend to reach for the callback process plugin at the drop of a hat. ;) I'm much more comfortable doing something directly in PHP than trying to chain a bunch of other processes together via the yml glue. But I definitely see value in this.

However, I'm curious how this will work in practice. Namely, how will other process plugins that expect a DOM object as input, not a string, advertise/declare themselves as such? And, for the chaining to really work, we'd need a way for a given DOM-aware plugin to know if it's the last step in the chain and should spit back HTML, or if it's going to keep being used and should pass on the modified DOM.

A brief skim of the patch doesn't reveal an obvious answer. Apologies if I'm missing something.

Thanks!
-Derek

Hi marvil07! It's been a while... ;)

Cool, thanks for clarifying. Makes sense. That'd work for me, but I presume this will need more fancy exception throwing in the general case before it could be officially adopted. Otherwise, I predict fairly interesting and useless error messages (if any) if someone got the details wrong. ;)

Just an FYI, my goto for HTML parsing has been https://packagist.org/packages/querypath/querypath, it's especially good if you're dealing with old-school HTML (no </p>, etc.).

A simplified example...

namespace Drupal\example_migrate\Plugin\migrate\process;

use Drupal\migrate\MigrateExecutableInterface;
use Drupal\migrate\ProcessPluginBase;
use Drupal\migrate\Row;

/**
 * Extract transcript HTML from a video node body.
 *
 * @MigrateProcessPlugin(
 *   id = "extract_transcript"
 * )
 */
class ExtractTranscript extends ProcessPluginBase {

  /**
   * {@inheritdoc}
   */
  public function transform($value, MigrateExecutableInterface $migrate_executable, Row $row, $destination_property) {
    // Extract the transcript, and remove the disclaimer.
    $qp_options = [
      'convert_to_encoding' => 'utf-8',
    ];
    $transcript = \QueryPath::withHTML($value, 'div.fpBody', $qp_options);
    $transcript->remove('p.legal');
    $transcript = $transcript->innerHTML();

    return $transcript;
  }

}

@dww, thanks for your comments!

Namely, how will other process plugins that expect a DOM object as input, not a string, advertise/declare themselves as such?

@marvil07 already answered this. I just want to point out that process plugins generally have no type hinting for the value they are given. It is typically a string or an array: often a nested array of strings. It is already the responsibility of the migration to chain process plugins together so that each provides the input expected from the next.

Otherwise, I predict fairly interesting and useless error messages (if any) if someone got the details wrong.

The import method already has

+    if (!is_string($value)) {
+      throw new MigrateException('Cannot import a non-string value.');
+    }

and the export method has

+    if (!$value instanceof \DOMDocument) {
+      $value_description = (gettype($value) == 'object') ? get_class($value) : gettype($value);
+      throw new MigrateException(sprintf('Cannot export a "%s".', $value_description));
+    }

Can you give a more specific suggestion on how to improve these? Should we do something other than throw MigrateException?

If you need to do custom processing with the DOMDocument class, then you can write a custom process plugin instead of a custom callback, and let this plugin handle converting from string to object and back.

Our plan is to have some configurable process plugins that do common transformations, like updating href and src attributes during migrations. @marvil07 has already added two other issues as children of the parent issue #2958642: DOM manipulation on process plugins, based on the code we are using on our current project.

Having a similar problem and wrote a class extending ProcessPluginBase for this.

I am also using regex in order to "translate" some xml elements into others as i couldn't find a smart way of doing it with DOM classes in php.

Also, the problem i am having is: i have an array of URLs i use to do the migration from... how can i access the "current" URL the transform method is running at?

thanks,

1. +++ b/src/Plugin/migrate/process/Dom.php
   @@ -0,0 +1,166 @@
   +  public function __construct(array $configuration, $plugin_id, $plugin_definition) {
   +    parent::__construct($configuration, $plugin_id, $plugin_definition);
   

   Let's throw a noisy exception if we don't pass a valid value for method or import or export.

2. +++ b/src/Plugin/migrate/process/Dom.php
   @@ -0,0 +1,166 @@
   +    // @todo It may be useful to have these parameters as configuration options.
   

   Then let's do this. An optional params option is a great idea.

3. +++ b/src/Plugin/migrate/process/Dom.php
   @@ -0,0 +1,166 @@
   +    if ($this->nonRoot) {
   

   We should also make this non root markup as an optional argument passed to the constructor.

4. +++ b/src/Plugin/migrate/process/Dom.php
   @@ -0,0 +1,166 @@
   +      set_error_handler(function(int $errno , string $errstr) use ($migrate_executable) {
   

   I don't think type hinting string and int is safe when using with php 5.6.

5. +++ b/tests/src/Unit/process/DomTest.php
   @@ -0,0 +1,65 @@
   +class DomTest extends MigrateProcessTestCase {
   

   Let's beef up the test case to include the edge cases I mention in my review previously.

@heddn, thanks for the detailed suggestions.

• 15.1 Done. I followed the example of the core callback plugin. Should we add a follow-up issue to do something similar in the skip_on_value plugin?
• 15.2 Done. I called the optional parameters "version" and "encoding", matching the documentation of the DOMDocument constructor. I also copied the descriptions from there. If that is not clear enough, then I could prefix these parameters with "domdocument_" or "xml_". What do you think?
• 15.3 Done. The default markup (now the html_template parameter) is complicated enough that I decided to add a defaultValues() method and simplify the handling of these defaults. I also added a second example in the documentation block, showing all the defaults.
• 15.4 I removed the type hints.
• 15.5 I added test coverage for validating the "method" parameter (15.1). Thanks to @tim.plunkett for his advice at #DrupalCampNJ. Did you have additional tests in mind?

This patch should fix the coding standards. I am also attaching an interdiff.

@dww:

In #6, you requested better error handling. I asked for more information in #12, but you did not reply. Maybe you are satisfied with the explanations @marvil07 and I gave, or with the validation I added in #16?

@joey-santiago:

Your question seems off topic for this issue. Maybe I am missing the connection. The transform() method (or the import() and export() methods in this issue) receive a $row parameter. Maybe the information you need is already there. If not, maybe you could add it in hook_migrate_prepare_row().

This should fix the test failures. (Make sure not to rely on optional parameters!) This change also gives migrations the option of specifying a custom html_template that (like the default) uses a token for the encoding.

I disagree with CodeSniffer about indentation. At the expense of adding a few lines, I can make a compromise that should satisfy us both.

1. +++ b/tests/src/Unit/process/DomTest.php
   @@ -0,0 +1,84 @@
   +    $document = (new Dom($configuration, 'dom', []))
   ...
   +    $document = (new Dom($configuration, 'dom', []))
   

   I don't think we need to assign to variable $document.

2. +++ b/tests/src/Unit/process/DomTest.php
   @@ -0,0 +1,84 @@
   +    $this->setExpectedException(MigrateException::class);
   ...
   +    $this->setExpectedException(MigrateException::class);
   

   Nit: Can we test the message for the exception?

@heddn:

Good catches. I also removed a $value = for the same reason as #25.1.

I'm doing a review of the doxygen now more thoroughly. Let's see if we can make the use of this process plugin a little easier.

1. +++ b/src/Plugin/migrate/process/Dom.php
   @@ -0,0 +1,232 @@
   + * - warnings_to_idmap: (optional) When parsing HTML, libxml may trigger
   + *   warnings. If this option is set to true, it will log them as migration
   + *   messages. Otherwise, it will not handle it in a special way. Only used if
   + *   method is 'import'. Defaults to true.
   

   Let's call this log_messages.

2. +++ b/src/Plugin/migrate/process/Dom.php
   @@ -0,0 +1,232 @@
   + * - version: (optional) The version number of the document as part of the XML
   + *   declaration. Only used if method is 'import'. Defaults to '1.0'.
   + * - encoding: (optional) The encoding of the document as part of the XML
   + *   declaration. Only used if method is 'import'. Defaults to 'UTF-8'.
   

   Is this also only important when non_root is false? Can we combine this with html_template and remove some config options?

3. +++ b/src/Plugin/migrate/process/Dom.php
   @@ -0,0 +1,232 @@
   + * - html_template: (optional) If non_root is true, then a complete HTML
   + *   document with the token '!value' to be replaced by the input text. You can
   + *   also use the token '!encoding'. Only used if method is 'import'. The
   + *   default has a DOCTYPE line, an <html> element, a <head> element, and a
   + *   <body> element containing the '!value' token.
   

   This is a complicated concept and I would like if we could make this easier to understand. Are these replacement values to make parsing the dom easier? Is there a more intuitive name for the config option than html_template? Like string_replacements, or something else?

More later, but for now:

In #15.3, you asked for the markup to be configurable. I realized that this was complicated, so I included the second example (specifying all optional parameters as their default values):

 *       html_template: |
 *         <!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd">
 *         <html xmlns="http://www.w3.org/1999/xhtml">
 *         <head><meta http-equiv="Content-Type" content="text/html; charset=utf-8" /></head>
 *         <body>!value</body>
 *         </html>

#27.1: Done. I made a similar change to the name of the corresponding class property.

If I were writing this from scratch, then I might not bother with the two class properties. I have kept them from the original patch.

#27.2: No, the version and encoding are also passed directly to the DOMDocument constructor:

    $document = new \DOMDocument($this->configuration['version'], $this->configuration['encoding']);

#27.3: I would rather remove this optional parameter than rename it. It is a wrapper for the supplied HTML fragment, so I do not think that string_replacements is a descriptive name.

Besides what I mentioned in #28, note that the import() and export() methods are basically wrappers for Html::load() and Html::serialize(), at least in the usual case where non_root is true (default). The export() method explicitly calls Html::serialize(). The only reason import() re-implements Html::load() instead of calling it is that we want to provide an option to log messages. (@marvil07 gets all the credit for that idea. We find it very helpful in debugging migrations.)

Maybe I should have pushed back on the suggestions from #15.2 and #15.3. We are providing options that may not be very useful and certainly are not provided by Html::load().

+++ b/src/Plugin/migrate/process/Dom.php
@@ -0,0 +1,234 @@
+ * - html_template: (optional) If non_root is true, then a complete HTML

I like you reasoning for removing this. Let's do that. It is complicated enough and if someone does need to adjust it, then can always extend this process plugin and make their own changes. Let's move some of these comments down into code where we are processing the html template and remove this config option.

Thanks for your patience.

Looks good now.

Thanks for all your contributions here.