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

Files: 
CommentFileSizeAuthor
#27 interdiff.txt4.66 KBbenjy
#27 2422227-27.patch7.75 KBbenjy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,875 pass(es). View
#26 2422227-25.patch7.45 KBbenjy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,339 pass(es). View
#25 interdiff.txt2.91 KBbenjy
#23 interdiff.txt2.11 KBbenjy
#23 2422227-23.patch6.4 KBbenjy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,174 pass(es). View
#20 interdiff.txt1.82 KBchx
#20 2422227_20.patch6.37 KBchx
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,239 pass(es). View
#16 2422227-16.patch5.83 KBbenjy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,229 pass(es). View
#13 interdiff.txt943 bytesbenjy

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
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,217 pass(es). View

Re-rolled.

benjy’s picture

FileSize
27.89 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,204 pass(es). View
2.77 KB

Added a merge flag.

benjy’s picture

FileSize
5.53 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,231 pass(es). View

Removed extra stuff in the last patch.

benjy’s picture

FileSize
7.71 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,226 pass(es). View
4.01 KB

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

benjy’s picture

FileSize
5.17 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,256 pass(es). View

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
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,888 pass(es). View
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
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,905 pass(es). View
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
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,189 pass(es). View
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
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,229 pass(es). View
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
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,230 pass(es), 1 fail(s), and 0 exception(s). View
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
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,290 pass(es). View

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

chx’s picture

FileSize
6.37 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,239 pass(es). View
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
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,174 pass(es). View
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
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,339 pass(es). View

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
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,339 pass(es). View

new patch. interdiff from last comment is correct.

benjy’s picture

FileSize
7.75 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,875 pass(es). View
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.