Problem/Motivation

Process plugin documentation is being moved from the Handbook to the API documentation of the process plugin class.

The handbook page for process plugin contains more information than the API documentation of the plugin.

Before we delete the handbook page and redirect it to the API page, we need to either enhance the API doc or decide that it is OK to discard the additional information currently found in the handbook.

The difference is this:
--clips--

YAML Booleans
Using an unquoted string which represents a boolean value in YAML (various forms of yes/no/true/false/on/off) as a key in your map will use that boolean value rather than the string itself as your key, which is almost certainly not what you want. Be sure to quote any such strings to use them as strings.

Doesn't work:

process:
  bar:
    plugin: static_map
    source: foo
    map:
      # This works as if it were '1: to'.
      TRUE: to

Does work:

process:
  bar:
    plugin: static_map
    source: foo
    map:
      'TRUE': to

Periods in key names
Mapping from a string which contains a period does not work at all:

process:
  bar:
    plugin: static_map
    source: foo
    map:
      'Version 3.0': version_3_0

This will give error:
exception 'Drupal\Core\Config\ConfigValueException' with message 'Version 3.0 key contains a dot which is not supported.' in /var/www/example/docroot/core/lib/Drupal/Core/Config/ConfigBase.php:211

There is an existing issue #2827897: static_map process plugin does not work with periods in keys to address this. In the meantime, a custom process plugin would be needed to map source data containing periods if they could not be removed by a previous process plugin like machine_name (which will run the value over the transliteration service, lowercases it, replaces anything that's not a number or a letter with an underscore and removes duplicate underscores):

process:
  bar:
    -
      plugin: machine_name
      source: foo
    -
      plugin: static_map
      map:
        'version_3_0': version_3_0

When writing the patch to include the additional documentation listed above, it was identified that there were several other areas needing improvements:

  • API documentation standards: how default values are indicated.
  • API documentation standards: use single quotes instead of double quotes.
  • It is very confusing which descriptive text belongs to which example.
  • There are many places where the phrasing can be clarified.

Proposed resolution

  • Include the 'YAML booleans' and 'periods in key names' topics in the API documentation
  • Fix API documentation standard violations.
  • Harmonize the descriptions so that the descriptive text is always above the code example.
  • Clarify the phrasing where possible

Remaining tasks

Do it.

User interface changes

N/A.

API changes

N/A.

Data model changes

N/A.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

masipila created an issue. See original summary.

masipila’s picture

masipila’s picture

Issue summary: View changes
Status: Active » Needs review
FileSize
4.64 KB

Initial patch attached. There were also multiple other issues which I fixed at the same time, see issue summary updates for details.

quietone’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/migrate/src/Plugin/migrate/process/StaticMap.php
    @@ -62,14 +63,11 @@
    + * unchanged, a 'bypass: true' option can be used. See the exmaple below. If the
    

    s/exmaple/example/

  2. +++ b/core/modules/migrate/src/Plugin/migrate/process/StaticMap.php
    @@ -81,11 +79,9 @@
    + * Default value can be defined for all values that are not included in the map.
    

    s/Default value/The default value/

  3. +++ b/core/modules/migrate/src/Plugin/migrate/process/StaticMap.php
    @@ -97,10 +93,37 @@
    + * Pay special attention with unquoted string representing a boolean value in
    

    Seems a bit long winded to get to the information. Can we make the first sentence a 'how to', i.e. 'How to map boolean values'.

  4. +++ b/core/modules/migrate/src/Plugin/migrate/process/StaticMap.php
    @@ -97,10 +93,37 @@
    + * The example below is incorrect ('TRUE' would be treated as '1'):
    

    I don't think an incorrect example is really useful here. Let's get rid of it and just show how to do it correctly.

  5. +++ b/core/modules/migrate/src/Plugin/migrate/process/StaticMap.php
    @@ -97,10 +93,37 @@
    + * Mapping from a string which contains a period does not work. A custom process
    

    I like the simplicity of this but it reads like it is a bug and makes me want to know why. I don't have a suggestion on how to improve it though.

  6. +++ b/core/modules/migrate/src/Plugin/migrate/process/StaticMap.php
    @@ -97,10 +93,37 @@
    + * @see https://www.drupal.org/project/drupal/issues/2827897
    
  7. Does this need an empty line before it?

Please check the indentation of "map:", it looks off in several of the existing examples.

masipila’s picture

Status: Needs work » Needs review
Issue tags: +Migrate January 2018 Sprint
FileSize
5.07 KB
3.73 KB

1. Fixed.

2. Fixed.

3-4. Fixed.

5. I rephrased this from 'not possible' to 'not supported'. The associated issue is linked with @see. My opinion is that we should include this in the API documentation as this is potentially a total WTF and therefore we should warn the developer about this. If we choose to include this remark in the API doc, then I will drop a note to the linked issue so that we will remember to remove the disclaimer when that issue will be fixed.

6. I'm not 100% sure. The API doc standards say this: Each @see reference is on its own line, with no additional text beyond the item being referenced. To provide more explanation, just use a regular documentation paragraph.

Markus

phenaproxima’s picture

Status: Needs review » Needs work

Couple of minor things, but mostly looks good to me.

  1. +++ b/core/modules/migrate/src/Plugin/migrate/process/StaticMap.php
    @@ -39,68 +41,74 @@
    + * The dfault value can be defined for all values that are not included in the
    

    Typo in "default". Also, it should be "A default value", not "The default value".

  2. +++ b/core/modules/migrate/src/Plugin/migrate/process/StaticMap.php
    @@ -39,68 +41,74 @@
    + * the value through the machine_name process plugin.
    + * @see https://www.drupal.org/project/drupal/issues/2827897
    

    Nit: There should be an extra blank line before the first @see.

masipila’s picture

Assigned: Unassigned » masipila
masipila’s picture

Assigned: masipila » Unassigned
Status: Needs work » Needs review
FileSize
5.07 KB
1.01 KB

1-2: Done.

quietone’s picture

Status: Needs review » Reviewed & tested by the community

The change to 'not supported' reads a whole lot better, thanks. I installed the patch and read through the whole doc. Everything in #4 and #7 has been addressed.

larowlan’s picture

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

8.4 is criticals only now

quietone’s picture

@larowlan, why the jump to 8.6.x?

larowlan’s picture

Adding review credits

larowlan’s picture

Version: 8.6.x-dev » 8.5.x-dev
Status: Reviewed & tested by the community » Fixed

Committed as 45662bb and pushed to 8.6.x.

Cherry-picked as eb9b03e and pushed to 8.5.x.

@quietone see https://groups.drupal.org/node/518148

Thanks folks

  • larowlan committed 45662bb on 8.6.x
    Issue #2933770 by masipila, quietone, phenaproxima: Merge handbook...

  • larowlan committed eb9b03e on 8.5.x
    Issue #2933770 by masipila, quietone, phenaproxima: Merge handbook...

Status: Fixed » Closed (fixed)

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