Add API documentation to the StaticMap process plugin.

CommentFileSizeAuthor
#14 2845489-14.patch3.49 KBjofitz
#14 interdiff-12-14.txt2.63 KBjofitz
#12 2845489-12.patch3.04 KBjofitz
#12 interdiff-9-12.txt3.04 KBjofitz
#9 2845489-9.patch3.97 KBjofitz
#9 interdiff-7-9.txt1.09 KBjofitz
#7 2845489-7.patch3.51 KBjofitz
#7 interdiff-2-7.txt2.63 KBjofitz
#2 2845489-1.patch3.47 KBquietone

Comments

quietone created an issue. See original summary.

quietone’s picture

quietone commented
Status: Active » Needs review
StatusFileSize
new 2845489-1.patch3.47 KB
phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
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.

ultimike’s picture

ultimike
Primary language English
Location Florida, USA
commented 
  1. +++ b/core/modules/migrate/src/Plugin/migrate/process/StaticMap.php
@@ -10,9 +10,76 @@
+ *  Available configuration keys
+ *    - source:
+ *    - map:
+ *    - bypass:

    default_value is also an available configuration key. 

  2. +++ b/core/modules/migrate/src/Plugin/migrate/process/StaticMap.php
@@ -10,9 +10,76 @@
+ * If the value of the source property foo was from then the value of the

    "from", "to", "this", "that" should probably be quoted.

  3. +++ b/core/modules/migrate/src/Plugin/migrate/process/StaticMap.php
@@ -10,9 +10,76 @@
+ * If the value of the source properties module and delta are filter and 2
+ * respectively, then the value of the destination id will be filter_url. By
+ * default, if a value is not found in the map, an exception is thrown. This is
+ * desirable in the above example. When static_map is used to just rename a few
+ * things and leave the others, a bypass: true option can be added. In this
+ * case, the source value is used unchanged.

    Again, example values should be quoted for better readability. 

  4. +++ b/core/modules/migrate/src/Plugin/migrate/process/StaticMap.php
@@ -10,9 +10,76 @@
+ * If the lookup fails, this value is used instead. Note that using the default

    Change "this value" to "this default_value"?

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs review » Needs work
jofitz’s picture

jofitz
he/him
commented
Assigned: phenaproxima » Unassigned
Status: Needs work » Needs review
StatusFileSize
new interdiff-2-7.txt2.63 KB
new 2845489-7.patch3.51 KB

Changes in response to code review.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs review » Needs work 
  1. +++ b/core/modules/migrate/src/Plugin/migrate/process/StaticMap.php
@@ -15,10 +15,11 @@
+ * Available configuration keys

    This needs to end with a colon. 

  2. +++ b/core/modules/migrate/src/Plugin/migrate/process/StaticMap.php
@@ -15,10 +15,11 @@
+ * - source:
+ * - map:
+ * - bypass:
+ * - default_value

    These are all missing descriptions.

jofitz’s picture

jofitz
he/him
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-7-9.txt1.09 KB
new 2845489-9.patch3.97 KB

Response to code review.

phenaproxima’s picture

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

Self-assigning for review.

phenaproxima’s picture

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

I'm sorry to say it, but I think this going to need some significant work.

  1. +++ b/core/modules/migrate/src/Plugin/migrate/process/StaticMap.php
@@ -10,9 +10,82 @@
+ * The static_map source plugin allows looking up a value based on a map
+ * specified in the configuration.

    Let's reword this a bit -- "Maps the input value to another value using an associative array specified in the configuration." 

  2. +++ b/core/modules/migrate/src/Plugin/migrate/process/StaticMap.php
@@ -10,9 +10,82 @@
+ * - map: An array (of 1 or more dimensions) that identifies the link between

    "the link" should read "the mapping". 

  3. +++ b/core/modules/migrate/src/Plugin/migrate/process/StaticMap.php
@@ -10,9 +10,82 @@
+ *   - TRUE: Do not throw an exception, return the source value.

    Let's strike the "Do not throw an exception" part; can this just say "Return the unmodified input value, or another default value if one is specified"? 

  4. +++ b/core/modules/migrate/src/Plugin/migrate/process/StaticMap.php
@@ -10,9 +10,82 @@
+ * respectively, then the value of the destination id will be "filter_url". By

    Should be "the returned value will be 'filter_url'". 

  5. +++ b/core/modules/migrate/src/Plugin/migrate/process/StaticMap.php
@@ -10,9 +10,82 @@
+ * default, if a value is not found in the map, an exception is thrown. This is
+ * desirable in the above example. When static_map is used to just rename a few

    Remove "This is desirable in the above example." It's not clear why it's desirable, and in an isolated example, it's not more or less desirable. 

  6. +++ b/core/modules/migrate/src/Plugin/migrate/process/StaticMap.php
@@ -10,9 +10,82 @@
+ * things and leave the others, a "bypass: true" option can be added. In this
+ * case, the source value is used unchanged.

    This should have an example of its own. 

  7. +++ b/core/modules/migrate/src/Plugin/migrate/process/StaticMap.php
@@ -10,9 +10,82 @@
+ * Also, a default_value can be provided.

    Redundant. Should be removed. 

  8. +++ b/core/modules/migrate/src/Plugin/migrate/process/StaticMap.php
@@ -10,9 +10,82 @@
+ * If the lookup fails, this default_value is used instead. Note that using the
+ * default value plugin after would not work as the pipeline has a value at this
+ * point: the value of the source specified for the static_map.

    Let's rewrite this entirely to explain the flow of the example, as the other explanations do. And let's not bother mentioning the default_value plugin or what the pipeline would contain -- it's out of scope for this documentation. 

  9. +++ b/core/modules/migrate/src/Plugin/migrate/process/StaticMap.php
@@ -21,7 +94,23 @@
+   * Performs the associated process.
+   *
+   * @param mixed $value
+   *   The input value.
+   * @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 string
+   *   The sub string of $value.
+   *
+   * @throws \Drupal\migrate\MigrateException
+   * @throws \Drupal\migrate\MigrateSkipRowException

    This should all be {@inheritdoc}.

jofitz’s picture

jofitz
he/him
commented
Assigned: phenaproxima » Unassigned
Status: Needs work » Needs review
StatusFileSize
new interdiff-9-12.txt3.04 KB
new 2845489-12.patch3.04 KB

Well that's the easy bits of #11 done: 1, 2, 3, 4, 5, 7, 9.

Remaining to do:

  1. +++ b/core/modules/migrate/src/Plugin/migrate/process/StaticMap.php
@@ -10,9 +10,82 @@
+ * things and leave the others, a "bypass: true" option can be added. In this
+ * case, the source value is used unchanged.

    This should have an example of its own. 

  2. +++ b/core/modules/migrate/src/Plugin/migrate/process/StaticMap.php
@@ -10,9 +10,82 @@
+ * If the lookup fails, this default_value is used instead. Note that using the
+ * default value plugin after would not work as the pipeline has a value at this
+ * point: the value of the source specified for the static_map.

    Let's rewrite this entirely to explain the flow of the example, as the other explanations do. And let's not bother mentioning the default_value plugin or what the pipeline would contain -- it's out of scope for this documentation.

jofitz’s picture

jofitz
he/him
commented
Status: Needs review » Needs work
jofitz’s picture

jofitz
he/him
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-12-14.txt2.63 KB
new 2845489-14.patch3.49 KB

Corrected some indentation in a previous example.

Addressed 11.6: Added a "bypass: true" example.

Addressed 11.8: Re-wrote the explanation for the last example.

quietone’s picture

quietone commented

@Jo Fitzgerald - The champion of process plugin documentation!

It looks like every in #11 has been addressed and it reads well, at least to me. I just have 2 questions:

+++ b/core/modules/migrate/src/Plugin/migrate/process/StaticMap.php
@@ -10,9 +10,98 @@
+ * Maps the input value to another value using an associative array specified in
+ * the configuration.

General considerations for API module parsing states "All summaries (first lines of docblocks) must be under 80 characters, start with a capital letter, and end with a period (.)." Maybe removing 'specified in the configuration'?

Why isn't it necessary to document the exceptions being thrown?

phenaproxima’s picture

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

Self-assigning for review.

phenaproxima’s picture

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

I tried to find problems with this documentation but I couldn't. It's really pretty clear. Great work!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Version: 8.4.x-dev » 8.3.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed 5bd7f43 and pushed to 8.4.x. Thanks!

Once 8.3.0 is out we can backport - we are in commit freeze atm.

  • alexpott committed 5bd7f43 on 8.4.x 
    Issue #2845489 by Jo Fitzgerald, quietone, phenaproxima, ultimike: Add...
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Patch (to be ported) » Fixed

Committed 478e2cd and pushed to 8.3.x. Thanks!

  • alexpott committed 478e2cd on 8.3.x 
    Issue #2845489 by Jo Fitzgerald, quietone, phenaproxima, ultimike: Add...

Status: Fixed » Closed (fixed)

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