Problem/Motivation

Right now, it is impossible to set NULL as default value for at least the extract migrate process plugin.

If one needs NULL as default value at the extract process plugin, the only workaround I've found is setting it to '' (empty string)|0 (zero integer)|FALSE etc and add a 'default_value' process plugin without 'strict' (or with strict set to FALSE, with NULL default value.

So, this does not work:

[...]
process:
  property:
    plugin: extract
    source: source_property
    index:
      - 0
      - value
    default_value: null
[...]

Workaround for the above:

[...]
process:
  property:
    -
      plugin: extract
      source: source_property
      index:
        - 0
        - value
      default_value: ''
    -
      plugin: default_value
      default_value: null
[...]

Proposed resolution

From discussion on Slack the way to go is to use array_key_exists.

These are the plugins with a default_value or default configuration property and all but Extract allow NULL to be a default value. This issue has a patch for Extract.

  1. /migrate/src/Plugin/migrate/process/DefaultValue.php
  2. migrate/src/Plugin/migrate/process/Extract.php
  3. migrate/src/Plugin/migrate/process/FormatDate.php
  4. migrate/src/Plugin/migrate/process/NullCoalesce.php
  5. migrate/src/Plugin/migrate/process/StaticMap.php
  • Create a change record.

    • Remaining tasks

    • Decide whether this is a bug, or a feature request. I'm unsure.
    • Assess BC implications.

    User interface changes

    API changes

    @tbd

    Data model changes

    Nothing.

    CommentFileSizeAuthor
    #16 3133516-16.patch4.94 KBquietone
    #16 interdiff-11-16.txt2.15 KBquietone
    #11 3133516-11.patch2.79 KBquietone
    #11 interdiff-5-11.patch2.28 KBquietone
    #11 3133516-11-fail.patch2.08 KBquietone
    #5 3133516-5.patch3.15 KBquietone
    #5 3133516-5-fail.patch1.99 KBquietone

    Comments

    huzooka created an issue. See original summary.

    huzooka’s picture

    huzooka
    Primary language Hungarian
    Location Hungary 🇭🇺🇪🇺
    commented

    null_coalesce and default_value are already working as expected.

    huzooka’s picture

    huzooka
    Primary language Hungarian
    Location Hungary 🇭🇺🇪🇺
    commented
    Issue summary: View changes
    wim leers’s picture

    wim leers
    Location Ghent 🇧🇪🇪🇺
    commented
    Issue tags: +migrate-d7-d8
    Related issues: +#3132101: Image media alt and title properties are not migrated to the corresponding image field properties

    Discovered in #3132101: Image media alt and title properties are not migrated to the corresponding image field properties.

    quietone’s picture

    quietone commented
    Status: Active » Needs review
    Issue tags: -migrate-d7-d8
    StatusFileSize
    new 3133516-5-fail.patch1.99 KB
    new 3133516-5.patch3.15 KB

    Since there is a workaround I am not too keen on this. On the other hand it is a bit jarring to have the extract default: behave differently than the default_value plugin. So, I made a patch.

    There are 23 migration yml files that use the extract plugin in core. A search of those did not find any that are followed by a default_value plugin for a destination property.

    Removing the migrate-d7-d8 tag since this is a process plugin.

    huzooka’s picture

    huzooka
    Primary language Hungarian
    Location Hungary 🇭🇺🇪🇺
    commented 
    +++ b/core/modules/migrate/tests/src/Unit/process/ExtractTest.php
@@ -55,4 +55,75 @@ public function testExtractFailDefault() {
+   * @param string|null $expected
+   *   The expected translformed value.

    I think that this should be tested with array and boolean types as well.
    s/translformed/transformed/

    The last submitted patch, 5: 3133516-5-fail.patch, failed testing. View results

    phenaproxima’s picture

    phenaproxima
    he/him
    Primary language English
    Location Massachusetts
    commented

    Since there is a workaround I am not too keen on this.

    There is a workaround, but it's undocumented and the existing behavior of extract is confusing and unexpected anyway. I think NULL is a perfectly legitimate default value to expect from extract, and from looking at the code, I suspect that the use of isset() was nothing more than an oversight.

    So, +1 for this generally. I'm not entirely sold on the approach of having extract wrap around default_value as it does here, but I could be convinced. :)

    quietone’s picture

    quietone commented

    @phenaproxima, I wrapped it around the default_value plugin to ensure that it always has the same behaviour. At the late hour when I did that it seemed the safer option than copying the code from default_value to extract.

    mikelutz’s picture

    mikelutz
    He/Him
    Primary language English
    Location Michigan, USA
    commented
    Status: Needs review » Needs work

    I'm not a fan either, Can we not just replace the isset check with a array_key_exists check and leave it at that?

    quietone’s picture

    quietone commented
    Status: Needs work » Needs review
    StatusFileSize
    new 3133516-11-fail.patch2.08 KB
    new interdiff-5-11.patch2.28 KB
    new 3133516-11.patch2.79 KB

    Sure can. It is now using an array_key_exists test. Since that changes the process plugin there is a new fail patch as well.

    quietone’s picture

    quietone commented
    Issue summary: View changes

    Ah, I see I am back to using the patch extension on diffs.

    Just reread the IS and it suggests that all process plugins that have a default property allow NULL to be set. The process plugins in core that have a default are:
    1 core/modules/migrate/src/Plugin/migrate/process/DefaultValue.php:
    2 core/modules/migrate/src/Plugin/migrate/process/Extract.php:
    3 core/modules/migrate/src/Plugin/migrate/process/FormatDate.php:
    4 core/modules/migrate/src/Plugin/migrate/process/NullCoalesce.php:
    5 core/modules/migrate/src/Plugin/migrate/process/StaticMap.php:

    I think they all allow NULL to be the default value. Would be helpful is someone double checked that.

    Edit: remove FormatDate

    The last submitted patch, 11: 3133516-11-fail.patch, failed testing. View results

    phenaproxima’s picture

    phenaproxima
    he/him
    Primary language English
    Location Massachusetts
    commented
    • DefaultValue accepts the default_value configuration option, and that option can be NULL.
    • FormatDate: I don't see anything in there to suggest that it accepts or returns NULL, or any other default value. It uses some default timezone stuff, but I don't think that's the same thing as we're dealing with here.
    • NullCoalesce accepts the default_value configuration option, but it must be non-NULL to be returned. IMHO we should change this one to array_key_exists() too, since NULL is a valid result from a regular null coalesce operation in PHP.
    • StaticMap accepts the default_value configuration option via array_key_exists(), which means it can return NULL.

    So: in HEAD right now, the behavior is inconsistent across plugins. We should change Extract and NullCoalesce to use array_key_exists(). I think we should also be sure that each of these plugins has test coverage of NULL handling.

    quietone’s picture

    quietone commented
    Status: Needs review » Needs work

    Sorry, I meant to remove FormatDate.

    Needs work NullCoalesce

    quietone’s picture

    quietone commented
    Status: Needs work » Needs review
    StatusFileSize
    new interdiff-11-16.txt2.15 KB
    new 3133516-16.patch4.94 KB

    NullCoalesce does not use array_key exists but it does return NULL if all the source values are NUL, and it even tells us that in the doc.

     * - default_value: (optional) The value to return if all values are NULL.
 *   if not provided, NULL is returned if all values are NULL.

    If someone does

       plugin: null_coalesce
   source:
     - NULL
     - NULL
   default_value: NULL

    The return value will be NULL, which is what is expected.

    Not sure we should change the code but I have added a test that I think makes that behavior a little clearer.

    mikelutz’s picture

    mikelutz
    He/Him
    Primary language English
    Location Michigan, USA
    commented
    Status: Needs review » Reviewed & tested by the community

    Agreed, Null Coalesce will return Null with no default value, and it will return it anyway if null is provided as default, though it wouldn't make sense to specify a default if you just wanted NULL to be the default.

    RTBC from me on this one.

    Status: Reviewed & tested by the community » Needs work

    The last submitted patch, 16: 3133516-16.patch, failed testing. View results

    quietone’s picture

    quietone commented
    Status: Needs work » Reviewed & tested by the community

    Random test failure a few days ago and retested. Tests are passing so back to RTBC

    alexpott’s picture

    alexpott
    he/they
    Primary language English
    Location 🇪🇺🌍
    commented
    Version: 9.1.x-dev » 9.0.x-dev
    Category: Feature request » Bug report
    Status: Reviewed & tested by the community » Fixed

    Committed and pushed ea0df4e831 to 9.1.x and 4768d66db0 to 9.0.x. Thanks!

    I think this is a bug not a feature request - you set the default value in the extract plugin to NULL and it doesn't work. This fixes that.

    • alexpott committed ea0df4e on 9.1.x 
      Issue #3133516 by quietone, huzooka, phenaproxima, mikelutz: Make every...

    • alexpott committed 4768d66 on 9.0.x 
      Issue #3133516 by quietone, huzooka, phenaproxima, mikelutz: Make every...

    Status: Fixed » Closed (fixed)

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