Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
Comment | File | Size | Author |
---|---|---|---|
#27 | interdiff.txt | 4.66 KB | benjy |
#27 | 2422227-27.patch | 7.75 KB | benjy |
#26 | 2422227-25.patch | 7.45 KB | benjy |
#25 | interdiff.txt | 2.91 KB | benjy |
#23 | interdiff.txt | 2.11 KB | benjy |
Comments
Comment #2
benjy CreditAttribution: benjy commentedRe-rolled.
Comment #3
benjy CreditAttribution: benjy commentedAdded a merge flag.
Comment #4
benjy CreditAttribution: benjy commentedRemoved extra stuff in the last patch.
Comment #5
benjy CreditAttribution: benjy commentedUseful improvements from @chx, added the hook calling as a static method on EntityStorageBase.
Comment #6
benjy CreditAttribution: benjy commentedPatch from the right issue this time.
Comment #7
chx CreditAttribution: chx commentedinstant like.
Comment #8
benjy CreditAttribution: benjy commentedSuggested 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.
Comment #9
chx CreditAttribution: chx commentedEven better.
Comment #10
alexpottWon't this break if someone does:
Comment #11
chx CreditAttribution: chx commentedI 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.)
Comment #12
benjy CreditAttribution: benjy commentedI think that will work fine.
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.
Comment #13
benjy CreditAttribution: benjy commentedThis normalizes the entire process array as suggested by @chx. That means a simple key: value is expanded into the full form:
Not sure if setting should be causing a re-normalize of every key though?
Comment #14
benjy CreditAttribution: benjy commentedI 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.
Comment #15
chx CreditAttribution: chx commentedThere 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.Comment #16
benjy CreditAttribution: benjy commentedPatch updated, not sure what you meant by normalizing on load but i've made the suggested changes.
Comment #17
chx CreditAttribution: chx commented> not sure what you meant by normalizing on load
This.
Comment #19
benjy CreditAttribution: benjy commentedRe-uploading, that test in 17 is our random test failure people have been reporting :)
Comment #20
chx CreditAttribution: chx commentedScratch 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.
Comment #21
chx CreditAttribution: chx commentedComment #22
dawehnerIn general its a nice cleanup.
Nitpick: missing space.
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?
Comment #23
benjy CreditAttribution: benjy commentedThanks for the review, new patch attached.
Comment #24
chx CreditAttribution: chx commented#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.Comment #25
benjy CreditAttribution: benjy commentedI 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.
Comment #26
benjy CreditAttribution: benjy commentednew patch. interdiff from last comment is correct.
Comment #27
benjy CreditAttribution: benjy commentedFurther renaming and doc improvements.
Comment #28
chx CreditAttribution: chx commentedRound #2 looks good.
Comment #29
webchickI had a few questions on this one, including:
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!