Add API documentation to the DefaultValue process plugin.

Comments

quietone created an issue. See original summary.

quietone’s picture

StatusFileSize
new1.65 KB
quietone’s picture

Status: Active » Needs review
phenaproxima’s picture

Assigned: Unassigned » phenaproxima

Self-assigning for review.

phenaproxima’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/migrate/src/Plugin/migrate/process/DefaultValue.php
    @@ -7,9 +7,38 @@
    + * value. The plugin sets the pipeline to the default value if the pipeline has
    + * no value (NULL, zero or empty string). The strict configuration key can be
    

    Let's remove these references to the pipeline -- they're confusing in the context of a single plugin. I think this should simply say "The plugin returns a default value if the input value is considered empty (NULL, FALSE, 0, '0', an empty string, or an empty array)".

  2. +++ b/core/modules/migrate/src/Plugin/migrate/process/DefaultValue.php
    @@ -7,9 +7,38 @@
    + * Available configuration keys
    

    Should end with a colon.

  3. +++ b/core/modules/migrate/src/Plugin/migrate/process/DefaultValue.php
    @@ -7,9 +7,38 @@
    + *     - false = apply default when pipeline value is empty().
    

    s/pipeline/input

  4. +++ b/core/modules/migrate/src/Plugin/migrate/process/DefaultValue.php
    @@ -7,9 +7,38 @@
    + *     - true = apply default when value is NULL.
    

    "Apply default when input value is NULL."

  5. +++ b/core/modules/migrate/src/Plugin/migrate/process/DefaultValue.php
    @@ -7,9 +7,38 @@
    + * Note that the pipeline starts with value NULL.
    

    Let's axe this sentence.

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.

gerzenstl’s picture

Status: Needs work » Needs review
StatusFileSize
new1.64 KB
new1.37 KB

Added changed to class documentation.

phenaproxima’s picture

Status: Needs review » Needs work

Looking really good. One very minor change, then it's RTBC.

+++ b/core/modules/migrate/src/Plugin/migrate/process/DefaultValue.php
@@ -10,16 +10,16 @@
+ * configuration key can be used to set the default only when the pipeline

Let's change "pipeline" to "incoming".

gerzenstl’s picture

Status: Needs work » Needs review
StatusFileSize
new1.64 KB
new1.37 KB

Change added.

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

Full steam ahead!

holingpoon’s picture

Edit: Please ignore this message. Just getting a hang of writing patches. Thanks!

I applied patch at #7 first before patch at #9. Should I have not done that? Because #9 came back with following error messages:

error: patch failed: core/modules/migrate/src/Plugin/migrate/process/DefaultValue.php:7
error: core/modules/migrate/src/Plugin/migrate/process/DefaultValue.php: patch does not apply

This is on 8.4.x branch.

The last submitted patch, 7: 2845477-7.patch, failed testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 9: 2845477-9.patch, failed testing.

gerzenstl’s picture

Version: 8.4.x-dev » 8.3.x-dev
quietone’s picture

Status: Needs work » Reviewed & tested by the community

Test really did pass.

xjm’s picture

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

As a docs improvement, this can go in patch releases.

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.

xjm’s picture

Status: Reviewed & tested by the community » Needs work

It'll be good to have these docs in the codebase. Just a couple small points:

  1. +++ b/core/modules/migrate/src/Plugin/migrate/process/DefaultValue.php
    @@ -7,9 +7,38 @@
    - * This plugin sets missing values on the destination.
    + * Sets a default value.
    

    Sets a default value for what?

  2. +++ b/core/modules/migrate/src/Plugin/migrate/process/DefaultValue.php
    @@ -7,9 +7,38 @@
    + * Available configuration keys:
    + *   - default_value: The fixed default value to apply.
    + *   - strict: (optional) Use strict value checking. Defaults to false.
    + *     - false = Apply default when input value is empty().
    + *     - true = Apply default when input value is NULL.
    

    The first list level is indented two spaces too far; the bullets should match the line above.

    Also, FALSE and TRUE should be all caps, and the = should be : instead to be formatted properly on API.d.o.

Thanks everyone!

jofitz’s picture

Status: Needs work » Needs review
StatusFileSize
new1.66 KB
new1.17 KB

Changes in response to code review.

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

Perfezione! (That's 'perfection' in Italian, according to Google Translate.)

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed 559e61a to 8.4.x and e2b7797 to 8.3.x. Thanks!

  • alexpott committed 559e61a on 8.4.x
    Issue #2845477 by gerzenstl, Jo Fitzgerald, quietone, phenaproxima, xjm...

  • alexpott committed e2b7797 on 8.3.x
    Issue #2845477 by gerzenstl, Jo Fitzgerald, quietone, phenaproxima, xjm...

Status: Fixed » Closed (fixed)

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