Add API documentation to the SkipOnEmpty process plugin.

CommentFileSizeAuthor
#28 interdiff-25-28.txt1.01 KBjofitz
#28 2845488-28.patch3.83 KBjofitz
#25 interdiff-24-25.txt616 bytesjofitz
#25 interdiff-17-25.txt2.23 KBjofitz
#25 2845488-25.patch3.92 KBjofitz
#24 interdiff-2845488-22-24.txt1.03 KByogeshmpawar
#24 add_documentation_to-2845488-24.patch3.9 KByogeshmpawar
#22 interdiff.txt762 bytesPavan B S
#22 2845488-22.patch3.94 KBPavan B S
#21 interdiff.txt1.45 KBgaurav.kapoor
#21 2845488-21.patch3.94 KBgaurav.kapoor
#17 interdiff-15-17.txt3.08 KBjofitz
#17 2845488-17.patch3.94 KBjofitz
#15 interdiff-9-15.txt2.58 KBjofitz
#15 2845488-15.patch4.01 KBjofitz
#13 interdiff-9-13.txt2.58 KBjofitz
#13 2845488-13.patch6.09 KBjofitz
#9 interdiff-1-9.txt3.99 KBgerzenstl
#9 2845488-9.patch4.65 KBgerzenstl
#7 interdiff-2845488-7.txt3.31 KBgerzenstl
#7 2845488-7.patch4.48 KBgerzenstl
#2 2845488-1.patch4.28 KBquietone
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

quietone created an issue. See original summary.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Status: Active » Needs review
FileSize
2845488-1.patch4.28 KB
phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Assigned: Unassigned » phenaproxima

Self-assigning for review.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Status: Needs review » Needs work
Issue tags: +SprintWeekend2017 
  1. +++ b/core/modules/migrate/src/Plugin/migrate/process/SkipOnEmpty.php
@@ -9,9 +9,89 @@
+ * Skips processing the current row when the source value evaluates to empty.

    s/source/input. And let's drop "evaluates to" and just change that to "is". 

  2. +++ b/core/modules/migrate/src/Plugin/migrate/process/SkipOnEmpty.php
@@ -9,9 +9,89 @@
+ * is empty (for example the empty string, FALSE, or 0). If so, the further

    Should be "(empty string, NULL, FALSE, 0, '0', or an empty array)". 

  3. +++ b/core/modules/migrate/src/Plugin/migrate/process/SkipOnEmpty.php
@@ -9,9 +9,89 @@
+ * To skip the processing of the property when an empty value is encountered,
+ * use the method: process setting.
+ * This is useful when migrating parent values. When the parent source value is
+ * 0 or empty, which represents the root element, there is no need to set a
+ * destination value. In the other case a parent exists and needs a migration
+ * lookup.
+ *
+ * To skip the entire row when an empty value is encountered, use the method:
+ * row setting.
+ * this is useful when combined with a migration process plugin to check if a
+ * related item was previously migrated.

    This should be reformulated as a listing of available configuration options. The only option this plugin exposes, AFAIK, is "method", so let's turn this into a cohesive description of that one option. 

  4. +++ b/core/modules/migrate/src/Plugin/migrate/process/SkipOnEmpty.php
@@ -9,9 +9,89 @@
+ * Example of usage for method: row:

    Let's delete this. 

  5. +++ b/core/modules/migrate/src/Plugin/migrate/process/SkipOnEmpty.php
@@ -9,9 +9,89 @@
+ * @code
+ * process:
+ *   field_type_exists:
+ *   -
+ *     plugin: migration
+ *     migration: d6_field
+ *     source:
+ *       - field_name
+ *   -
+ *     plugin: skip_on_empty
+ *     method: row
+ *   -
+ *     plugin: extract
+ *     index:
+ *       - 1
+ * @endcode

    Add a couple of sentences here describing what the effect of this code will be. 

  6. +++ b/core/modules/migrate/src/Plugin/migrate/process/SkipOnEmpty.php
@@ -9,9 +9,89 @@
+ * Examples of usage for method:  process:

    Delete this. 

  7. +++ b/core/modules/migrate/src/Plugin/migrate/process/SkipOnEmpty.php
@@ -9,9 +9,89 @@
+ * @code
+ * process:
+ *   parent:
+ *   -
+ *     plugin: skip_on_empty
+ *     method: process
+ *     source: parent
+ *   -
+ *     plugin: migration
+ *     migration: d6_taxonomy_term
+ * @endcode

    Again, these examples should explain themselves afterwards. 

  8. +++ b/core/modules/migrate/src/Plugin/migrate/process/SkipOnEmpty.php
@@ -20,7 +100,22 @@
+   * Performs the associated process.

    This is not accurate. The method actually throws a MigrateSkipRowException if the value is empty -- this should be documented. 

  9. +++ b/core/modules/migrate/src/Plugin/migrate/process/SkipOnEmpty.php
@@ -20,7 +100,22 @@
+   * @throws \Drupal\migrate\MigrateSkipRowException

    Need to document the reason why the exception is thrown. 

  10. +++ b/core/modules/migrate/src/Plugin/migrate/process/SkipOnEmpty.php
@@ -30,7 +125,22 @@ public function row($value, MigrateExecutableInterface $migrate_executable, Row
+   * Performs the associated process.

    As with row(), this needs to be more specific. 

  11. +++ b/core/modules/migrate/src/Plugin/migrate/process/SkipOnEmpty.php
@@ -30,7 +125,22 @@ public function row($value, MigrateExecutableInterface $migrate_executable, Row
+   * @throws \Drupal\migrate\MigrateSkipProcessException

    Needs to document the reason why the exception is thrown.

gerzenstl’s picture

gerzenstl CreditAttribution: gerzenstl at 42mate commented
Assigned: phenaproxima » gerzenstl
gerzenstl’s picture

gerzenstl CreditAttribution: gerzenstl at 42mate commented
Version: 8.4.x-dev » 8.3.x-dev
Status: Needs work » Needs review
FileSize
2845488-7.patch4.48 KB
interdiff-2845488-7.txt3.31 KB

@phenaproxima, point #11 of your suggested changes cannot be added for the moment, MigrateSkipProcessException() extends from Exception class but currently is an empty class. Maybe the only thing we can mention about this exception is "This exception is thrown when the rest of the process should be skipped."

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Status: Needs review » Needs work

Most exception classes are empty subclasses of Exception. :) It should simply say "thrown if the source property is not set" or something like that.

gerzenstl’s picture

gerzenstl CreditAttribution: gerzenstl at 42mate commented
Status: Needs work » Needs review
FileSize
2845488-9.patch4.65 KB
interdiff-1-9.txt3.99 KB

I agree, comments added.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Status: Needs review » Needs work

Thank you, @gerzenstl. Great work :) There are a couple of nitpicks, but also some more serious problems that need to be addressed (by a maintainer or at least an experienced Migrate developer, so no worries if you don't want to tackle these).

  1. +++ b/core/modules/migrate/src/Plugin/migrate/process/SkipOnEmpty.php
@@ -9,9 +9,87 @@
+ * is empty (empty string, NULL, FALSE, 0, '0', or an empty array). If so, the further

    This line exceeds 80 characters. 

  2. +++ b/core/modules/migrate/src/Plugin/migrate/process/SkipOnEmpty.php
@@ -9,9 +9,87 @@
+ *     - row: Skips the entire row when an empty value is encountered. Useful when

    Exceeds 80 characters. 

  3. +++ b/core/modules/migrate/src/Plugin/migrate/process/SkipOnEmpty.php
@@ -9,9 +9,87 @@
+ *   -
+ *     plugin: extract
+ *     index:
+ *       - 1

    Let's get rid of this, it just confuses the example. 

  4. +++ b/core/modules/migrate/src/Plugin/migrate/process/SkipOnEmpty.php
@@ -9,9 +9,87 @@
+ * If field_name is empty, skips the row process.

    Can we change "skips the row process" to "skips the entire row"? 

  5. +++ b/core/modules/migrate/src/Plugin/migrate/process/SkipOnEmpty.php
@@ -9,9 +9,87 @@
+ * If parent is empty, skips parent process but d6_taxonomy_term will be
+ * processed.

    wat. This is confusing...and I'm not sure it's accurate, either. Surely, if the processing is skipped on empty, the migration plugin will not run..?! 

  6. +++ b/core/modules/migrate/src/Plugin/migrate/process/SkipOnEmpty.php
@@ -9,9 +9,87 @@
+ * @code
+ *  process:
+ *    book.pid:
+ *    -
+ *      plugin: skip_on_empty
+ *      method: process
+ *      source: plid
+ *    -
+ *     plugin: migration
+ *     migration: d6_book
+ * @endcode
+ *
+ * or
+ *
+ * @code
+ *  process:
+ *    pid:
+ *    -
+ *      plugin: skip_on_empty
+ *      method: process
+ *      source: pid
+ *    -
+ *      plugin: migration
+ *      migration: d6_comment
+ * @endcode

    Both of these examples need a further comment explaining what will happen in each case, assuming it's different than the example above. Otherwise, can remove them both.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented 
+++ b/core/modules/migrate/src/Plugin/migrate/process/SkipOnEmpty.php
@@ -9,9 +9,87 @@
+ *        encountered. Useful when migrating parent values, there is no need to
+ *        set a destination value.

"Useful when migrating parent values, there is no need to set a destination value." I don't understand what this means. I think we either need to clarify it, or strike it entirely.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Version: 8.3.x-dev » 8.2.x-dev
jofitz’s picture

jofitz CreditAttribution: jofitz at ComputerMinds commented
Status: Needs work » Needs review
FileSize
2845488-13.patch6.09 KB
interdiff-9-13.txt2.58 KB

Documentation tweaks in response to #10/11.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Status: Needs review » Needs work

The latest patch includes the changes to the Concat plugin. I think we need to reroll without those :)

jofitz’s picture

jofitz CreditAttribution: jofitz at ComputerMinds commented
Assigned: gerzenstl » Unassigned
Status: Needs work » Needs review
FileSize
2845488-15.patch4.01 KB
interdiff-9-15.txt2.58 KB

Got my git branches confused! The less said about that the better.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Status: Needs review » Needs work 
  1. +++ b/core/modules/migrate/src/Plugin/migrate/process/SkipOnEmpty.php
@@ -9,9 +9,53 @@
+ *   -
+ *     plugin: migration
+ *     migration: d6_field
+ *     source:
+ *       - field_name
+ *   -
+ *     plugin: skip_on_empty
+ *     method: row
+ * @endcode
+ *
+ * If field_name is empty, skips the entire row.

    The call to the migration plugin here is incorrect. It should be removed entirely. Should be:

    field_type_exists:
  plugin: skip_on_empty
  method: row
  source: field_name

  2. +++ b/core/modules/migrate/src/Plugin/migrate/process/SkipOnEmpty.php
@@ -9,9 +9,53 @@
+ * If parent is empty, skips parent process.

    Let's rephrase this. "If parent is empty, any further processing of the property is skipped. In this example, if parent is empty, the next plugin (migration) will not be run." 

  3. +++ b/core/modules/migrate/src/Plugin/migrate/process/SkipOnEmpty.php
@@ -20,7 +64,25 @@
+   * Skips migration process at row level when value is not set and throws a
+   * MigrateSkipRowException with STATUS_IGNORED status.

    This is too long. It should just be "Skips the current row when value is not set." 

  4. +++ b/core/modules/migrate/src/Plugin/migrate/process/SkipOnEmpty.php
@@ -20,7 +64,25 @@
+   * @return string
+   *   The sub string of $value.

    This function doesn't return anything. 

  5. +++ b/core/modules/migrate/src/Plugin/migrate/process/SkipOnEmpty.php
@@ -20,7 +64,25 @@
+   *   Thrown if source property is not set and row should be skipped, by
+   *    default records with STATUS_IGNORED status in the map.

    Nit: the indentation should be consistent. And let's get rid of "by default records with STATUS_IGNORED status in the map", it makes no sense. 

  6. +++ b/core/modules/migrate/src/Plugin/migrate/process/SkipOnEmpty.php
@@ -30,7 +92,25 @@ public function row($value, MigrateExecutableInterface $migrate_executable, Row
+   * Skips migration process at process level when value is not set and throws a
+   * MigrateSkipProcessException.

    Too long. Should be "Stops processing the current property when value is not set." 

  7. +++ b/core/modules/migrate/src/Plugin/migrate/process/SkipOnEmpty.php
@@ -30,7 +92,25 @@ public function row($value, MigrateExecutableInterface $migrate_executable, Row
+   * @return string
+   *   The sub string of $value.

    This method has no return value. 

  8. +++ b/core/modules/migrate/src/Plugin/migrate/process/SkipOnEmpty.php
@@ -30,7 +92,25 @@ public function row($value, MigrateExecutableInterface $migrate_executable, Row
+   *   Thrown if source property is not set and rest of the process should be
+   *    skipped.

    Nit: inconsistent indentation.

jofitz’s picture

jofitz CreditAttribution: jofitz at ComputerMinds commented
Status: Needs work » Needs review
FileSize
2845488-17.patch3.94 KB
interdiff-15-17.txt3.08 KB

I have addressed 1, 2, 3, 5, 6, 8 form #16 (and another inconsistent indentation not mentioned), but disagree with 4 & 7 - in both cases if $value is 'falsey' then the input value will be returned, i.e. there is a return value. I have edited the documentation to reflect this.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Murphysboro, IL, USA
CreditAttribution: mikeryan as a volunteer and at Virtuoso Performance commented
Assigned: Unassigned » mikeryan
mikeryan’s picture

mikeryan
he/him
Primary language English
Location Murphysboro, IL, USA
CreditAttribution: mikeryan as a volunteer and at Virtuoso Performance commented
Assigned: mikeryan » Unassigned
Status: Needs review » Needs work 
  1. +++ b/core/modules/migrate/src/Plugin/migrate/process/SkipOnEmpty.php
@@ -9,9 +9,50 @@
+ *   parent:
+ *   -
+ *     plugin: skip_on_empty
+ *     method: process
+ *     source: parent
+ *   -
+ *     plugin: migration
+ *     migration: d6_taxonomy_term

    Contents of parent: need another level of indentation. 

  2. +++ b/core/modules/migrate/src/Plugin/migrate/process/SkipOnEmpty.php
@@ -9,9 +9,50 @@
+ * If parent is empty, any further processing of the property is skipped. In
+ * this example, if parent is empty, the next plugin (migration) will not be
+ * run.

    This paragraph sounds a bit redundant. Maybe something like "If parent is empty, any further processing of the property is skipped - thus, the next plugin (migration) will not be run." 

  3. +++ b/core/modules/migrate/src/Plugin/migrate/process/SkipOnEmpty.php
@@ -20,7 +61,24 @@
+   *   The input value, $value, if it is not falsey.

@@ -30,7 +88,24 @@ public function row($value, MigrateExecutableInterface $migrate_executable, Row
+   *   The input value, $value, if it is not falsey.

    I don't see any usage of "falsey" in core - let's just use "empty". 

  4. +++ b/core/modules/migrate/src/Plugin/migrate/process/SkipOnEmpty.php
@@ -20,7 +61,24 @@
+   *   Thrown if source property is not set and row should be skipped, by

    s/source property/the source property/, s/row/the row/ - articles help! 

  5. +++ b/core/modules/migrate/src/Plugin/migrate/process/SkipOnEmpty.php
@@ -20,7 +61,24 @@
+   *   default records with STATUS_IGNORED status in the map.

    Since there's no way to record anything other than STATUS_IGNORED, I'd omit "by default". 

  6. +++ b/core/modules/migrate/src/Plugin/migrate/process/SkipOnEmpty.php
@@ -30,7 +88,24 @@ public function row($value, MigrateExecutableInterface $migrate_executable, Row
+   *   Thrown if source property is not set and rest of the process should be

    Articles!

Thanks!

gaurav.kapoor’s picture

gaurav.kapoor CreditAttribution: gaurav.kapoor at OpenSense Labs commented
Status: Needs work » Needs review
FileSize
2845488-21.patch3.94 KB
interdiff.txt1.45 KB
Pavan B S’s picture

Pavan B S CreditAttribution: Pavan B S at Valuebound commented
FileSize
2845488-22.patch3.94 KB
interdiff.txt762 bytes 
+++ b/core/modules/migrate/src/Plugin/migrate/process/SkipOnEmpty.php
@@ -30,7 +88,24 @@ public function row($value, MigrateExecutableInterface $migrate_executable, Row
+   *   Thrown if the source property is not set and rest of the process should be

Line is exceeding 80 characters

jofitz’s picture

jofitz CreditAttribution: jofitz at ComputerMinds commented
Status: Needs review » Needs work

Well that's half the job...

Still need to address #20.1, #20.2 & #20.5.

yogeshmpawar’s picture

yogeshmpawar
He/Him/His
Primary language English
Location 🇮🇳 Pune(MH)
CreditAttribution: yogeshmpawar at QED42 commented
Status: Needs work » Needs review
FileSize
add_documentation_to-2845488-24.patch3.9 KB
interdiff-2845488-22-24.txt1.03 KB

Changes done as per suggestions in #20 & also added a interdiff.

jofitz’s picture

jofitz CreditAttribution: jofitz at ComputerMinds commented
FileSize
2845488-25.patch3.92 KB
interdiff-17-25.txt2.23 KB
interdiff-24-25.txt616 bytes

Let's push this one over the line.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Assigned: Unassigned » phenaproxima

Self-assigning for review.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Assigned: phenaproxima » Unassigned
Status: Needs review » Needs work 
  1. +++ b/core/modules/migrate/src/Plugin/migrate/process/SkipOnEmpty.php
@@ -9,9 +9,49 @@
+ * Available configuration keys:
+ *   - method: (optional) level of skip.
+ *     - row: Skips the entire row when an empty value is encountered. Useful
+ *        when combined with a migration process plugin to check if a related
+ *        item was previously migrated.
+ *     - process: Skips the processing of the property when an empty value is
+ *        encountered.

    Whoa, this indentation is pretty wonky. The documentation for method should be flush with the word "Available". Also, "level of skip" should be "What to do if the input value is empty. Possible values:" 

  2. +++ b/core/modules/migrate/src/Plugin/migrate/process/SkipOnEmpty.php
@@ -9,9 +9,49 @@
+ *     - row: Skips the entire row when an empty value is encountered. Useful
+ *        when combined with a migration process plugin to check if a related
+ *        item was previously migrated.

    I'd prefer to remove the mention of the migration process plugin here. It's out of scope. 

  3. +++ b/core/modules/migrate/src/Plugin/migrate/process/SkipOnEmpty.php
@@ -9,9 +9,49 @@
+ *     - process: Skips the processing of the property when an empty value is
+ *        encountered.

    Should be "Prevents further processing of the input property when the value is empty".

jofitz’s picture

jofitz CreditAttribution: jofitz at ComputerMinds commented
Status: Needs work » Needs review
FileSize
2845488-28.patch3.83 KB
interdiff-25-28.txt1.01 KB

Fixed errors identified in #27.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Status: Needs review » Reviewed & tested by the community

Hell yeah!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed fed0b7d to 8.4.x and 3e272aa to 8.3.x. Thanks!

  • alexpott committed fed0b7d on 8.4.x 
    Issue #2845488 by Jo Fitzgerald, gerzenstl, Pavan B S, Yogesh Pawar,...

  • alexpott committed 3e272aa on 8.3.x 
    Issue #2845488 by Jo Fitzgerald, gerzenstl, Pavan B S, Yogesh Pawar,...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

rajeevku’s picture

rajeevku CreditAttribution: rajeevku commented
Issue tags: -SprintWeekend2017