Problem/Motivation

We already have code like this and it's only going to become more common for contrib modules that need to add their own process. Eg:

    $process = $migration->getProcess();
    $process[$field_name] = [
      'plugin' => 'd6_cck_file',
      'source' => [
        $field_name,
        $field_name . '_list',
        $field_name . '_data',
      ],
    ];
    $migration->setProcess($process);

Proposed resolution

Add a setProcessOfProperty() and a mergeProcessOfProperty to the Migration entity to make this simpler. Also, make sure that getProcess returns a normalized process for alters and such.

Remaining tasks

Review

User interface changes

API changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

The last submitted patch, set-process-property.patch, failed testing.

benjy’s picture

Status: Needs work » Needs review
FileSize
4.52 KB

Re-rolled.

benjy’s picture

FileSize
27.89 KB
2.77 KB

Added a merge flag.

benjy’s picture

FileSize
5.53 KB

Removed extra stuff in the last patch.

benjy’s picture

FileSize
7.71 KB
4.01 KB

Useful improvements from @chx, added the hook calling as a static method on EntityStorageBase.

benjy’s picture

FileSize
5.17 KB

Patch from the right issue this time.

chx’s picture

Status: Needs review » Reviewed & tested by the community
-    $process = $migration->getProcess();

instant like.

benjy’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
5.74 KB
3.7 KB

Suggested feedback from @alexpott. The set method now simply sets the process for the given field name.

New method mergeProcessOfProperty merges the incoming process array explicitly without a merge flag. If the field doesn't already exist in $this->process it falls back to a set.

chx’s picture

Status: Needs review » Reviewed & tested by the community

Even better.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review

Won't this break if someone does:

$this->setProcessOfProperty('foo', 'bar');
$this->mergeProcessOfProperty('foo', ['baz']);
chx’s picture

I guess we can add a check if set tries to set something that isnt an array and run the normalizer or whatever it is called over it (is it called processProcess? if so that'd be hilarious. It's in the entity class.)

benjy’s picture

I think that will work fine.

$this->setProcessOfProperty('foo', 'bar');
// Performs, $this->process['foo'] = 'bar';

$this->mergeProcessOfProperty('foo', ['baz']);
// Performs
$this->process = NestedArray::mergeDeepArray([$this->process, ['foo' => ['baz']]], TRUE);

Because the keys are merged for process, this should work just fine. It won't try to merge a non array value with an array, simply overwrite.

benjy’s picture

FileSize
5.89 KB
943 bytes

This normalizes the entire process array as suggested by @chx. That means a simple key: value is expanded into the full form:

key:
  plugin: get
  source: value

Not sure if setting should be causing a re-normalize of every key though?

benjy’s picture

FileSize
5.94 KB
1.1 KB

I removed the normalizing from merge, I don't think that made any sense.

I've added a is_string around $process which means the entire process only gets normalized if you try to set a non-normalized value.

chx’s picture

There is a conundrum here which always rears its ugly head when we have alters over a structure that can have various internal formats . This is merely the symptom, not the cause. Since getProcessNormalized is cheap I recommend normalizing on load. And then on set you can do $this->process = $this->getProcessNormalized([$field_name => $process] + $this->process to keep it normalized and then on merge there's no question as to what to pass in since you know that merge is normalized so $this->process = NestedArray::mergeDeep($this->getProcessNormalized([$field_name => $process]), $this->process) is ought to work. alter() will also know that $this->process is normalized.

benjy’s picture

FileSize
5.83 KB
1.29 KB

Patch updated, not sure what you meant by normalizing on load but i've made the suggested changes.

chx’s picture

FileSize
6.59 KB
1.05 KB

> not sure what you meant by normalizing on load

This.

Status: Needs review » Needs work

The last submitted patch, 17: 2422227_17.patch, failed testing.

benjy’s picture

Status: Needs work » Needs review
FileSize
6.59 KB

Re-uploading, that test in 17 is our random test failure people have been reporting :)

chx’s picture

FileSize
6.37 KB
1.82 KB

Scratch that. The interdiff is against #16. This patch normalizes on get so that outside callers always have the normalized process. Since process plugins are cached anyways the performance penalty is negliglible, the normalizer is not heavy.

This makes normalization in setProcessOfProperty unnecessary, alters unambigous and merges have no problems since we merge normalized to normalized. The on load approach rubbed as not-the-best-but-I-do-not-have-a-better-idea. Well, now I have.

chx’s picture

Issue summary: View changes
dawehner’s picture

In general its a nice cleanup.

  1. +++ b/core/modules/migrate/src/Entity/Migration.php
    @@ -406,6 +410,31 @@ public function setProcess(array $process) {
    +    //otherwise simply set it.
    

    Nitpick: missing space.

  2. +++ b/core/modules/migrate/src/Entity/MigrationInterface.php
    @@ -198,6 +198,36 @@ public function getProcess();
    +   * @param mixed $process
    +   *   A valid process value, array or string.
    ...
    +  public function setProcessOfProperty($field_name, $process);
    +
    ...
    +   * @param array $process
    +   *   An array of process data.
    ...
    +  public function mergeProcessOfProperty($field_name, array $process);
    

    It is a bit confusing that $process can be both an array and mixed. Can you maybe clarify that with better variable names? Individual vs. all process information?

benjy’s picture

FileSize
6.4 KB
2.11 KB

Thanks for the review, new patch attached.

chx’s picture

#22 raises a great question, one of the hard problems: naming things. Before these methods came to existence, $process was always a variable containing every property. But these particular methods are talking about defining the process pipeline for a single property. What about using that word? I am a little hesitant because so far the only mention of pipelines are in the getProcessPlugins doxygen saying 'Each pipeline contains an array of plugins." so then perhaps -- pipeline_configuration? Should we rename the methods as well? Or keeping with the method name call it $process_of_property? Perhaps that would be easiest. And add the pipeline word to the doxygen. And while we are at it, what about extending the description of getProcess from "The configuration describing the process plugins." to say something about it being normalized? That would be a great way to sneak in the shorthands description from https://www.drupal.org/node/2129651 into core finally.

benjy’s picture

FileSize
2.91 KB
6.4 KB

I tried to improve the comments, interdiff against #20. I couldn't think of a good variable name before but as chx suggested, $process_of_property works well for me.

I updated a few comments but i'm open to improvements.

benjy’s picture

FileSize
7.45 KB

new patch. interdiff from last comment is correct.

benjy’s picture

FileSize
7.75 KB
4.66 KB

Further renaming and doc improvements.

chx’s picture

Status: Needs review » Reviewed & tested by the community

Round #2 looks good.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

I had a few questions on this one, including:

@@ -77,6 +78,9 @@ class Migration extends ConfigEntityBase implements MigrationInterface, Requirem
   /**
    * The configuration describing the process plugins.
    *
+   * This is a strictly internal property and should not returned to calling
+   * code, use getProcess() instead.
+   *
    * @var array
    */
   protected $process;

Why do we use protected here when according to the docs we mean private? ("There's a language feature for that.") Benjy explained that this is because sometimes you want to swap out the migrate entity entirely, and this wouldn't work if the variable were private. (And Migrate Drupal module, for example, actually does this.)

Also confused why we have both a setProcessOfProperty() and a mergeProcessOfProperty() when merge is capable of setting, and then developers don't need to learn two similar-but-different methods. But according to benjy this split was requested by alexpott. Fair enough.

Committed and pushed to 8.0.x. Thanks!

  • webchick committed 518eff9 on 8.0.x
    Issue #2422227 by benjy, chx, dawehner: Add setProcessOfProperty to...

Status: Fixed » Closed (fixed)

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