While working on a migration with an image field inside of a Multifield I've been struggling with an issue with migrating to the image field. The full description of the problem is in that issue, but here it is again.
In using a multifield, if you want to set a field subclass (e.g. file_class, preserve_files, source_type, etc), you have to do it in three levels:
$this->addFieldMapping('field_images:field_image', 'field_image')
->sourceMigration('CNSFile');
$this->addFieldMapping('field_images:field_image:file_class')
->defaultValue('MigrateFileFid');
$this->addFieldMapping('field_images:field_image:preserve_files')
->defaultValue(TRUE);
However, the code in Migration::applyMappings() doesn't handle this because it uses hardcoded values:
// Are we dealing with the primary value of the destination field, or a
// subfield?
$destination = explode(':', $destination);
$destination_field = $destination[0];
if (isset($destination[1])) {
$subfield = $destination[1];
// We're processing the subfield before the primary value, initialize it
if (!property_exists($this->destinationValues, $destination_field)) {
$this->destinationValues->$destination_field = array();
}
// We have a value, and need to convert to an array so we can add
// arguments.
elseif (!is_array($this->destinationValues->$destination_field)) {
$this->destinationValues->$destination_field = array($this->destinationValues->$destination_field);
}
// Add the subfield value to the arguments array.
$this->destinationValues->{$destination_field}['arguments'][$subfield] = $destination_values;
}
// Just the primary value, the first time through for this field, simply
// set it.
elseif (!property_exists($this->destinationValues, $destination_field)) {
$this->destinationValues->$destination_field = $destination_values;
}
// We've seen a subfield, so add as an array value.
else {
$this->destinationValues->{$destination_field} = array_merge(
(array)$destination_values, $this->destinationValues->{$destination_field});
}
Since it only looks for $destination[0] and $destination[1] (and doesn't look to see if there is $destination[2]), when it gets to this line:
// Add the subfield value to the arguments array.
$this->destinationValues->{$destination_field}['arguments'][$subfield] = $destination_values;
it overwrites the desired data value in $this->destinationValues->{$destination_field}['arguments'][$subfield] ($this->destinationValues->{'field_images'}['arguments']['field_image] in my case) with the subclass value, in my case overwriting a file id first with the MigrateFileFid class name, and then preserve_files value (true). So, when it gets to the multifield migration handler, the field has a value of true instead of the fid, that part of the migration fails, and I get an SQL error about inserting a blank value into the file_usage.fid field.
So as I see it, this method could be improved by being able to handle more than two levels of values in the field mappings for cases like this. Assuming that is ok, I'd be more than happy to write a patch, but if not, I'm open to suggestions.
Thanks.
| Comment | File | Size | Author |
|---|---|---|---|
| #5 | migrate-applyMappings-three-levels-2403643.patch | 2.77 KB | wonder95 |
Comments
Comment #1
mikeryanInteresting, haven't seen multifield before. To be clear, the context for this is #2217353: Migrate field handler (patch), there's a patch for migration support in multifield, but it seems difficult for the contrib module to access the extra layer of subfield without a change to Migrate.
So, by the criteria for feature requests going forward, this seems to fit #3 (not easily achievable through the existing API), but not so much #2 (narrow use case) - I prefer not to build support for specific contrib modules into Migrate itself, and multifield is at this point not that widely used and still at an alpha release. I'm not closing the door entirely, but I'd want to be sure any patch is minimally disruptive, and that all avenues for doing this completely on the multifield end are explored (could anything be done through a destination handler prepare()?).
Comment #2
wonder95 commentedWell, in the case of Multifield, it was done specifically as a field handler, and not a destination handler (like the Migrate handler for Field Collection - the one you helped me out with at Drupalcon Austin - is). Speaking from experience, Multifield is much better because it is a field, and not a separate entity.
My train of thought is to modify applyMappings() so that it can handle either case (2 or 3 subfields), and it would work just as it does now for 2. For now, I'm just overriding applyMappings() and the Multifield prepare handler in my own class, and if I get something to work that is generic, I'll post it here.
Comment #3
mikeryanI didn't mean a destination handler instead of a field handler, but in addition - I think it should be possible to use the destination handler to parse out the extra layer of subfield and get things in place for the field handler to pick up. It'll probably be ugly, though...
Comment #4
wonder95 commentedMike,
I'm not as familiar with the handler calling order as you are, so can you clarify for me how this would work? The problem that I run into with applyMappings() is that by the time the data gets to the field handler, the data for the third level has been stomped on, so it's not even available. How would this work with adding a destination handler?
Thanks.
Comment #5
wonder95 commentedSo I finally got things working between the two modules, and it requires small patches on both sides. Attached is a first attempt at a patch to Migration::applyMappings() that handles three levels of subfields and puts data and arguments appropriately in $this->destinationValues->{$destination_field}. This requires some small changes in the multifield handler, but together they work perfectly. I don't think this change falls under the category of making a change for a specific module - who knows that another field module might not come along that requires three levels - and it certainly doesn't disrupt anything for regular fields, but that's your call to make.
Comment #6
mikeryanI'm a little leery of messing with a central (and complex) piece of Migrate in the short release cycle for 2.7, especially for a narrow use case. Once 2.7 is released I'll commit this so we have the full 2.8 release cycle for it to get exercised.
Comment #7
t0xicCode commentedAny movement on this? I'd be willing to work on re-rolling the patch and testing it as needed.
Comment #8
wonder95 commented@t0xicCode, I used this for a large migration with 7.x-2.7, and it worked great.
Comment #9
mikeryanNeeds to have the "Needs review" status for the testbot to take a look at it...
Comment #11
mikeryanI've gone ahead and committed this, thanks.