Rename Iterator process plugin and add documentation

Self-assigning for review.

Patch reviewed and tested , uploaded with correct format.

@gaurav.kapoor, there's no diff between #2 and #4. Why are you reposting the same patch?

And the patch was assigned for review to @phenaproxima, you cannot RTBC it.

1. +++ b/core/modules/migrate/src/Plugin/migrate/process/Iterator.php
   @@ -7,9 +7,116 @@
   + * (for simple lists see handling multiple values). Often source data will
   

   "see handling multiple values" should be removed, or at least further explained, because it seems like it might be a link that was pasted in from the handbook page.

2. +++ b/core/modules/migrate/src/Plugin/migrate/process/Iterator.php
   @@ -7,9 +7,116 @@
   + * logically include multiple-value properties as associative arrays. For
   

   Let's get rid of "logically".

3. +++ b/core/modules/migrate/src/Plugin/migrate/process/Iterator.php
   @@ -7,9 +7,116 @@
   + * Available configuration keys
   

   Should have a colon at the end.

4. +++ b/core/modules/migrate/src/Plugin/migrate/process/Iterator.php
   @@ -7,9 +7,116 @@
   + *   - process:
   

   Er...needs an explanation.

5. +++ b/core/modules/migrate/src/Plugin/migrate/process/Iterator.php
   @@ -7,9 +7,116 @@
   + * element [0], [1], etc. in the source filters field, and apply the static_map
   

   Let's get rid of [0], [1], etc. -- it's confusing.

6. +++ b/core/modules/migrate/src/Plugin/migrate/process/Iterator.php
   @@ -7,9 +7,116 @@
   + * The map source keys are module and delta, so in the map key, since the
   + * [module] value is filter, we index the array using the delta value of 2 and
   + * arrive at filter_url as our value - this will be the first value in the array
   + * assigned to the destination filters. Similarly, we will take the 0 value for
   + * the weight in the second array to generate a second value of filter_html_escape.
   + *
   + * There's one more problem that needs to be solved: often the key of the arrays
   + * need to be changed. In this case, the new id property needs to be the key.
   + * It'd be possible to just add key: "@id" to the iterator process but that
   + * would require special casing the key property and somehow escape it when
   + * there is actually a destination called key. Instead, it is moved out of the
   + * process and key is in the configuration of iterator itself:
   

   This is so convoluted and strangely worded as to be borderline incomprehensible. I think it should be completely rewritten, mostly by the use of examples. This is the sort of plugin that benefits from concrete examples, not complicated and wordy explanations. I can take a stab at this later.

7. +++ b/core/modules/migrate/src/Plugin/migrate/process/Iterator.php
   @@ -7,9 +7,116 @@
   + * The process pipeline for key is special in two ways: first, it runs after
   + * the normal process is done. Because of this, key: "@key" is also valid.
   + * Second, the pipeline does not start with NULL but the value of the current key.
   + * This way, the value of key can come from the other properties or the current
   + * key. For the latter, omit the source from the first plugin in the pipeline
   + * for key.
   

   We should rewrite this as part of the rest of the rewrite.

8. +++ b/core/modules/migrate/src/Plugin/migrate/process/Iterator.php
   @@ -7,9 +7,116 @@
   + * An example of the input and output data is attached.
   

   This should go. Nothing is attached.

1, 2, 3, 5, 8 from comment No. 8 is now complete. Please refer to files 2845483-11.patch and interdiff-2845483-1-11.txt. This issue still needs more work as some of the documentation needs to be rewritten.

This looks good so far.

Addressed #8.4 and corrected some indentation (which had me totally confused until I realised it was an error!)

I can take a stab at this later.

I'll leave @phenaproxima to have a go at the hell fun that is #8.6 and #8.7!

Setting back to "Needs work" so someone (else) can address #8.6 & #8.7:

6. +++ b/core/modules/migrate/src/Plugin/migrate/process/Iterator.php
   @@ -7,9 +7,116 @@
   + * The map source keys are module and delta, so in the map key, since the
   + * [module] value is filter, we index the array using the delta value of 2 and
   + * arrive at filter_url as our value - this will be the first value in the array
   + * assigned to the destination filters. Similarly, we will take the 0 value for
   + * the weight in the second array to generate a second value of filter_html_escape.
   + *
   + * There's one more problem that needs to be solved: often the key of the arrays
   + * need to be changed. In this case, the new id property needs to be the key.
   + * It'd be possible to just add key: "@id" to the iterator process but that
   + * would require special casing the key property and somehow escape it when
   + * there is actually a destination called key. Instead, it is moved out of the
   + * process and key is in the configuration of iterator itself:

   This is so convoluted and strangely worded as to be borderline incomprehensible. I think it should be completely rewritten, mostly by the use of examples. This is the sort of plugin that benefits from concrete examples, not complicated and wordy explanations. I can take a stab at this later.

7. +++ b/core/modules/migrate/src/Plugin/migrate/process/Iterator.php
   @@ -7,9 +7,116 @@
   + * The process pipeline for key is special in two ways: first, it runs after
   + * the normal process is done. Because of this, key: "@key" is also valid.
   + * Second, the pipeline does not start with NULL but the value of the current key.
   + * This way, the value of key can come from the other properties or the current
   + * key. For the latter, omit the source from the first plugin in the pipeline
   + * for key.

   We should rewrite this as part of the rest of the rewrite.

This is an entirely reworked set of docs; hopefully making things clearer. It uses a different example entirely in hopes it is more simple.

I'd also propose a rename from iterator to sub_process. At least that's what I'm looking for votes for or against. Or alternate names.

An here's the rename.

Copying from IRC, there were votes in favor of sub_process/SubProcess from @phenaproxima and @mikeryan.

1. +++ b/core/modules/migrate/src/Plugin/migrate/process/SubProcess.php
   @@ -0,0 +1,139 @@
   + * Processes a row of data as a sub-process.
   

   Sub-processing is extremely hard to sum up in less than 80 characters, but nonetheless, I wonder if there is a better way to phrase this.

2. +++ b/core/modules/migrate/src/Plugin/migrate/process/SubProcess.php
   @@ -0,0 +1,139 @@
   + * The sub_process process plugin allows sub-processing of a list of associative
   + * arrays. Often source data will include multiple-value properties as
   + * associative arrays. For example, for file uploads on Drupal 6, the upload
   + * property contains a list of sub-properties.
   

   I feel like we could sum this up better with something like this: "The sub_process plugin accepts an array of associative arrays and runs each one through its own process pipeline, producing a new associative array of transformed values."

3. +++ b/core/modules/migrate/src/Plugin/migrate/process/SubProcess.php
   @@ -0,0 +1,139 @@
   + *   - key: runs the process pipeline for the defined key.
   

   I'm not 100% sure this is accurate. Doesn't it actually specify which key of the transformed row will be the key of that row in the sub_process plugin's output?

4. +++ b/core/modules/migrate/src/Plugin/migrate/process/SubProcess.php
   @@ -0,0 +1,139 @@
   + * In this case, each item in the upload array will be processed bye the
   

   "Bye Bye Bye" is now stuck in my head :) Or, for those unfamiliar with '90s boy bands, this should be "processed by".

5. +++ b/core/modules/migrate/src/Plugin/migrate/process/SubProcess.php
   @@ -0,0 +1,139 @@
   + * The mapping values passed to sub_process are special. Numeric values may not
   + * be used.
   + *
   + * @code
   + *   field_relationship:
   +       -
   +         plugin: migration_lookup
   + *       migration:
   + *         - migration_page
   + *         - migration_blog
   + *       source: nid
   + *     -
   + *       plugin: sub_process
   + *       process:
   + *         target_id: '0'
   + * @endcode
   + * In the migration above, the value mapped to target_id is intentionally '0'.
   

   I don't understand this. What is a "mapping value" in the context of sub_process?

6. +++ b/core/modules/migrate/src/Plugin/migrate/process/SubProcess.php
   @@ -0,0 +1,139 @@
   +  /**
   +   * Runs a process pipeline on each destination property per list item.
   +   */
   

   Should be @inheritdoc.

7. +++ b/core/modules/migrate/src/Plugin/migrate/process/SubProcess.php
   @@ -0,0 +1,139 @@
   +    if (!is_null($value)) {
   

   $value not being null does not guarantee that $value is iterable. We should change this line to if (is_array($value) || $value instanceof \Traversable)

8. +++ b/core/modules/migrate/tests/src/Unit/process/SubProcessTest.php
   @@ -42,15 +42,15 @@ public function testIterator() {
   +      ->will($this->returnValue($sub_process_plugins));
   

   Nit -- Can we just change this to ->willReturn($sub_process_plugins)?

#23.5: I removed that from the docs. After reviewing my notes and the code, this is actually a limitation of the get process plugin. Not of sub_process.

All the rest of the points are strait forward and addressed.

Totally getting there. It's improving each time, and this is by far the hardest plugin to explain in a succinct manner.

1. +++ b/core/modules/migrate/src/Plugin/migrate/process/SubProcess.php
   @@ -7,16 +7,15 @@
   + * Processes associative array value of a row through its own process pipeline.
   

   Can we rephrase this even more, to "Runs an array of arrays through a process pipeline."

2. +++ b/core/modules/migrate/src/Plugin/migrate/process/SubProcess.php
   @@ -7,16 +7,15 @@
   + *   - key: runs the process pipeline for the key to determine its dynamic name.
   

   I still don't understand this. I think it needs to be illustrated with an explicit example.

I am almost totally satisfied with this :) @heddn, you are a champ. I have but one complaint before being ready to RTBC.

+++ b/core/modules/migrate/src/Plugin/migrate/process/SubProcess.php
@@ -63,6 +64,48 @@
+ *     process:
+ *       id:
+ *         plugin: static_map
+ *         bypass: true
+ *         map:
+ *           filter:
+ *             ...

I think this example could be a bit clearer. Can we change it to something more concrete, like an example involving concat, so that it's obvious what the generated key would be?

process:
  id:
    plugin: concat
    source:
      - module
      - delta
    delimiter: __

In this example, the keys of the returned array would be filter__2 and filter__0.

I think you uploaded the wrong patch. :)

Here's the right patch now.

Er...#30 seems to be missing the changes I see in the interdiff from #26?

/me blushes.

This pleases me.

Thanks folks.

I checked that the depreciation implementation is right. Looked around existing depreciated classes in migrate which have this trigger_error in core. I did not find the docs on how to do this but this is consistent with the other existing implementations of keeping the class around and its docs as it was.

The expanded docs look great, good job IMHO :)

I've published the change record.

Tagging for release notes.