Add documentation to Explode process plugin

Self-assigning for review.

1. +++ b/core/modules/migrate/src/Plugin/migrate/process/Explode.php
   @@ -8,9 +8,39 @@
   + * Explodes the source value into an array of values.
   

   s/source value/input value

2. +++ b/core/modules/migrate/src/Plugin/migrate/process/Explode.php
   @@ -8,9 +8,39 @@
   + * The explode process plugin creates an array of strings, each of which is a
   + * substring formed by splitting the source on boundaries formed by the string
   + * delimiter. One use case is for building the theme and color configuration
   

   Can this simply say "This plugin splits a string into an array of strings by a delimiter", or similar?

3. +++ b/core/modules/migrate/src/Plugin/migrate/process/Explode.php
   @@ -8,9 +8,39 @@
   + * setting name from the Drupal 7 variable name. For example, to convert
   + * theme_bartik_settings to bartik.settings.
   

   It's not clear how to use this plugin to transform theme_bartik_settings to bartik.settings. I think this example should be clarified.

4. +++ b/core/modules/migrate/src/Plugin/migrate/process/Explode.php
   @@ -8,9 +8,39 @@
   + * Available configuration keys
   

   Should end with a colon.

5. +++ b/core/modules/migrate/src/Plugin/migrate/process/Explode.php
   @@ -8,9 +8,39 @@
   + *   - source: The source string.
   

   Should say "the input string".

6. +++ b/core/modules/migrate/src/Plugin/migrate/process/Explode.php
   @@ -8,9 +8,39 @@
   + *     - If limit is set and positive, the returned array will contain a maximum of limit elements with the last element containing the rest of string.
   + *     - If the limit parameter is negative, all components except the last -limit are returned.
   

   These lines exceed 80 characters.

7. +++ b/core/modules/migrate/src/Plugin/migrate/process/Explode.php
   @@ -8,9 +8,39 @@
   + *   - delimiter: (optional) The boundary string.
   

   String explosion uses a delimiter. It's not clear, therefore, why the delimiter could be optional. We should explain this better.

8. +++ b/core/modules/migrate/src/Plugin/migrate/process/Explode.php
   @@ -8,9 +8,39 @@
   + * If limit is > PHP_INT_MAX it will be set to PHP_INT_MAX.
   

   I don't think we need to mention this. It's the most exotic edge case imaginable.

+++ b/core/modules/migrate/src/Plugin/migrate/process/Explode.php
@@ -19,7 +49,23 @@
+   * Performs the associated process.
+   *
+   * @param string $value
+   *   The input string.
+   * @param \Drupal\migrate\MigrateExecutableInterface $migrate_executable
+   *   The migration in which this process is being executed.
+   * @param \Drupal\migrate\Row $row
+   *   The row from the source to process.
+   * @param string $destination_property
+   *   The destination property currently worked on. This is only used together
+   *   with the $row above.
+   *
+   * @return array
+   *   The array of sub strings created by splitting the input on boundaries
+   *   formed by the string delimiter.
+   *
+   * @throws \Drupal\migrate\MigrateException

This should all be @inheritdoc.

Added interdiff.

After I talked with @phenaproxima, I added some changes on the initial explanation.

Also I removed the "(optional)" for the delimiter parameter. It's mandatory, otherwise, it throws an exception on line #63.

Looking better and better. I'd still like to see a more solid example, but we can leave that for a Migrate maintainer.

One nitpick beyond that:

+++ b/core/modules/migrate/src/Plugin/migrate/process/Explode.php
@@ -8,21 +8,23 @@
+ *     - If limit is set and negative, all components except the last -limit are returned.

This line exceeds 80 characters.

Fixed line.

All looks good :)

Although I would suggest moving line 42 ("If limit is > PHP_INT_MAX it will be set to PHP_INT_MAX.") into the available configuration keys block. This would put this information along with the other options/limits of the 'limit' key and would put the example configuration next to its equivalent PHP code.

Leaving as "Needs review" as other people may disagree with me.

1. +++ b/core/modules/migrate/src/Plugin/migrate/process/Explode.php
   @@ -8,9 +8,42 @@
   + * One use case is for building the theme and color configuration
   + * setting name from the Drupal 7 variable name. For example, to convert
   + * theme_bartik_settings to bartik.settings.
   

   I'm not a big fan of this example use case. I often encounter migrations where a source field contains a comma-separated values list of term names or list item values. These need to be exploded to an array to be migrated.

2. +++ b/core/modules/migrate/src/Plugin/migrate/process/Explode.php
   @@ -8,9 +8,42 @@
   + * If limit is > PHP_INT_MAX it will be set to PHP_INT_MAX.
   

   I agree with @phenaproxima's earlier comment - this is such an edge case it can probably be eliminated.

   As an aside, I'd **really** like to see an additional parameter (boolean) that allows the user to specify if each array value should be trimmed. If I wrote a patch for this, any chance it would be accepted?

One small thing. Other than that, this is good to go.

+++ b/core/modules/migrate/src/Plugin/migrate/process/Explode.php
@@ -8,9 +8,47 @@
+ * If foo is "node/1/edit", then bar will be ['node', '1/edit']. The PHP
+ * equivalent of this would be: $bar = explode('/', $foo)

It would actually be $bar = explode('/', $foo, 1).

Looks perfect.

Straightforward code documentation improvements can always go into any patch release, alpha, beta, or RC, so please always file them against the production branch (currently 8.3.x). Thanks!

It might be better to avoid calling this process plugin with the same name of a php function...

Setting back to RTBC from testbot error.

@giorgio79, it's fine for a plugin to have the same name as a PHP function. The plugin name is just a label and the internals work through a class. As can be seen from the new documentation, it works it has the same effect as the function so the name does not cause confusion.

This one is very close!

@John Cook is correct; plugin machine names and PHP functions don't need to be unique from each other. Also, note that this is a documentation issue, so discussing the naming that already exists is out of scope for this patch.

1. +++ b/core/modules/migrate/src/Plugin/migrate/process/Explode.php
   @@ -8,9 +8,47 @@
   + * Available configuration keys:
   + *   - source: The source string.
   + *   - limit: (optional)
   + *     - If limit is set and positive, the returned array will contain a maximum
   + *        of limit elements with the last element containing the rest of string.
   + *     - If limit is set and negative, all components except the last -limit are
   + *        returned.
   + *     - If the limit parameter is zero, then this is treated as 1.
   + *   - delimiter: The boundary string.
   

   Same note as elsewhere about list indentation; the top level needs to have the hyphens at the same indentation as the paragraph above. Believe it or not, this is a coding standard and core has been patched before only to fix such list indentation.

2. +++ b/core/modules/migrate/src/Plugin/migrate/process/Explode.php
   @@ -8,9 +8,47 @@
   + * If foo is "node/1", then bar will be ['node', '1']. The PHP equivalent of
   + * this would be: $bar = explode('/', $foo).
   ...
   + * If foo is "node/1/edit", then bar will be ['node', '1/edit']. The PHP
   + * equivalent of this would be: $bar = explode('/', $foo, 1).
   

   As on the other issue, let's put full lines of PHP (that are actual code rather than just a variable name) in their own @code with complete syntax.

Thanks!

Corrections in response to code review.

1. +++ b/core/modules/migrate/src/Plugin/migrate/process/Explode.php
   @@ -14,14 +14,14 @@
   + *   - If limit is set and positive, the returned array will contain a maximum
   + *      of limit elements with the last element containing the rest of string.
   

   I think the "of" is indented one space too far...? Fixable on commit.

2. +++ b/core/modules/migrate/src/Plugin/migrate/process/Explode.php
   @@ -14,14 +14,14 @@
   + *   - If limit is set and negative, all components except the last -limit are
   + *      returned.
   

   Ditto with "returned".

Indeed, #26 fixed on commit. The diff:

diff --git a/core/modules/migrate/src/Plugin/migrate/process/Explode.php b/core/modules/migrate/src/Plugin/migrate/process/Explode.php
index 9b41ebe8ea..ef7c94aabf 100644
--- a/core/modules/migrate/src/Plugin/migrate/process/Explode.php
+++ b/core/modules/migrate/src/Plugin/migrate/process/Explode.php
@@ -17,9 +17,9 @@
  * - source: The source string.
  * - limit: (optional)
  *   - If limit is set and positive, the returned array will contain a maximum
- *      of limit elements with the last element containing the rest of string.
+ *     of limit elements with the last element containing the rest of string.
  *   - If limit is set and negative, all components except the last -limit are
- *      returned.
+ *     returned.
  *   - If the limit parameter is zero, then this is treated as 1.
  * - delimiter: The boundary string.
  *

There is now #2852163: Lists in comments not validated to add a rule that should help catch this whitespace issue in the future.

Committed to 8.4.x and cherry-picked to 8.3.x. Thanks everyone!