Add API documentation to the SkipRowifNotSet process plugin.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

quietone created an issue. See original summary.

quietone’s picture

Status: Active » Needs review
FileSize
0 bytes
quietone’s picture

The last submitted patch, 2: 2845492-1.patch, failed testing.

phenaproxima’s picture

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

Status: Needs review » Needs work
Issue tags: +SprintWeekend2017
  1. +++ b/core/modules/migrate/src/Plugin/migrate/process/SkipRowIfNotSet.php
    @@ -8,9 +8,27 @@
    + * The skip_row_if_not_set process plugin checks whether a value is set. If the
    + * value is set the value is returned otherwise a MigrateSkipRowException
    + * is thrown.
    

    Let's rephrase this: "If the value is set, it is returned. Otherwise, a MigrateSkipRowException is thrown."

  2. +++ b/core/modules/migrate/src/Plugin/migrate/process/SkipRowIfNotSet.php
    @@ -8,9 +8,27 @@
    + * Available configuration keys
    

    Should end with a colon.

  3. +++ b/core/modules/migrate/src/Plugin/migrate/process/SkipRowIfNotSet.php
    @@ -8,9 +8,27 @@
    + *   - index: The source value to test.
    

    This should be "The source property to check for."

  4. +++ b/core/modules/migrate/src/Plugin/migrate/process/SkipRowIfNotSet.php
    @@ -8,9 +8,27 @@
    + * Example
    

    Should end with a colon.

  5. +++ b/core/modules/migrate/src/Plugin/migrate/process/SkipRowIfNotSet.php
    @@ -8,9 +8,27 @@
    + * @code
    + *  process:
    + *    settings:
    + *    # Check if the "contact" key exists in the "data" array.
    + *    plugin: skip_row_if_not_set
    + *    index: contact
    + *    source: data
    + * @endcode
    

    This should have an explanation afterwards to demonstrate what output would be expected.

  6. +++ b/core/modules/migrate/src/Plugin/migrate/process/SkipRowIfNotSet.php
    @@ -20,7 +38,22 @@
    +   * 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\MigrateSkipRowException
    

    This should be {@inheritdoc}.

FJ7’s picture

Status: Needs work » Needs review
FileSize
1.7 KB
phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/core/modules/migrate/src/Plugin/migrate/process/SkipRowIfNotSet.php
@@ -19,9 +40,10 @@
+  ¶
+   /**
+    * {@inheritdoc}
+    */

There's a little extra white space on the blank line above the comment, but that can be fixed on commit.

Otherwise, looks good. RTBC from me on the assumption that the tests will pass.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/migrate/src/Plugin/migrate/process/SkipRowIfNotSet.php
@@ -8,9 +8,27 @@
- * @link https://www.drupal.org/node/2345935 Online handbook documentation for skip_row_if_not_set process plugin @endlink

Why are we removing the link - it is still valid no?

phenaproxima’s picture

Status: Needs work » Reviewed & tested by the community

We discussed this on IRC, and I think we reached a general consensus that documentation for individual process plugins should be kept in the API, where it will be most up-to-date and easily linkable from the handbook pages.

The handbook, for its part, will get all of its pages describing individual process plugins ripped out, replaced with a general overview of how the process plugins work, how they fit together in a pipeline, and how to use them in a migration. We'll link to the individual API pages for each process plugin. Therefore, the links are essentially no longer valid and should not be in the doc comments.

xjm’s picture

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

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!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 8: 2845492.patch, failed testing.

jofitz’s picture

Assigned: phenaproxima » Unassigned
Status: Needs work » Reviewed & tested by the community

Test-bot error, returning to RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed 24a4d4b to 8.4.x and 789fd08 to 8.3.x. Thanks!

diff --git a/core/modules/migrate/src/Plugin/migrate/process/SkipRowIfNotSet.php b/core/modules/migrate/src/Plugin/migrate/process/SkipRowIfNotSet.php
index ce89bba..d2e4089 100644
--- a/core/modules/migrate/src/Plugin/migrate/process/SkipRowIfNotSet.php
+++ b/core/modules/migrate/src/Plugin/migrate/process/SkipRowIfNotSet.php
@@ -40,10 +40,9 @@
  */
 class SkipRowIfNotSet extends ProcessPluginBase {
 
-  
-   /**
-    * {@inheritdoc}
-    */
+  /**
+   * {@inheritdoc}
+   */
   public function transform($value, MigrateExecutableInterface $migrate_executable, Row $row, $destination_property) {
     if (!isset($value[$this->configuration['index']])) {
       throw new MigrateSkipRowException();

Fixed on commit.

  • alexpott committed 24a4d4b on 8.4.x
    Issue #2845492 by quietone, FJ7, phenaproxima: Add documentation to...

  • alexpott committed 789fd08 on 8.3.x
    Issue #2845492 by quietone, FJ7, phenaproxima: Add documentation to...

Status: Fixed » Closed (fixed)

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