Problem/Motivation
When running migration upgrades for contrib fields, the formatters are not mapped during the upgrade. Essentially, the formatter mappings defined in MigrateFieldInterface::getFieldFormatterMap()
have no effect. This is due to incorrect mappings constructed/merged in MigrateFieldInterface::processFieldFormatter()
.
The problem is that d6_field_formatter_settings
and d7_field_formatter_settings
don't use the same number of sources for the "options/type" property and thus their static maps don't have the same number of levels.
d6_field_formatter_settings
"options/type":
-
plugin: static_map
bypass: true
source:
- type
- 'display_settings/format'
map:
number_integer:
default: number_integer
[...]
d7_field_formatter_settings
"options/type":
-
plugin: static_map
bypass: true
source: '@formatter_type'
map:
date_default: datetime_default
[...]
And the code that merges the formatter maps from the field plugins assume that the static maps have two levels. This works for Drupal 6 but not for Drupal 7.
FieldPluginBase::processFieldFormatter()
foreach ($this->getFieldFormatterMap() as $source_format => $destination_format) {
$process[0]['map'][$plugin_id][$source_format] = $destination_format;
}
$migration->mergeProcessOfProperty('options/type', $process);
Proposed resolution
Modify the d7_field_formatter_settings
to use a two level static map like in d6_field_formatter_settings
. We also moved all the content of the d7_field_formatter_settings
"options/type" static map in the respective field plugins' getFieldFormatterMap()
because that's cleaner.
To make sure that all core field formatter types are correctly mapped and tested, we opened a follow-up: #2944604: [META] Check that all field formatter types are mapped and tested.
We also opened another follow-up to make sure that all core field widget types are correctly mapped and tested: #2944605: [META] Check that all field widget types are mapped and tested.
Remaining tasks
Patch
Test
Review
User interface changes
None
API changes
None
Data model changes
None
Comment | File | Size | Author |
---|---|---|---|
#34 | interdiff-2843617-32-34.txt | 806 bytes | maxocub |
#34 | 2843617-34.patch | 12.53 KB | maxocub |
#32 | interdiff-2843617-27-32.txt | 3 KB | maxocub |
#32 | 2843617-32.patch | 12.36 KB | maxocub |
#27 | interdiff-2843617-24-27.txt | 9.99 KB | maxocub |
Comments
Comment #2
hussainwebAttached a patch with the fix.
Comment #4
hussainwebIt seems this has opened a can of worms. All the errors I saw so far (I just took a glance) seems to be about incorrect count of migrated field instances. It seems we are now able to migrate more? I'll need to look into this further.
Comment #6
quietone CreditAttribution: quietone as a volunteer commentedJust noting what I found poking at this today.
In d6_field_formatter_settings.yml the input to static_map is a 2 dimensional array but in d7_field_formatter_settings.yml it is a string. However, in CckFieldPluginBase::processFieldFormatter, which adds entries to that static_map, entries are made for the d6 version. Therefor all the static map entries made by processFieldFormatter for d7 are simply ignored. That would explain what hussainweb experienced.
And it is further confused because some of the Cck field plugins use the same getFieldFormatter code for d6 and d7, which makes one think they are the same.
My first thought on a fix is to make the d7 migration like the d6 one. There is also, probably, cleanup to do to make the d7 getFieldFormatters return an empty array.
On the right track?
Comment #7
quietone CreditAttribution: quietone as a volunteer commentedSo, in d7 getFieldFormatterMap adds items to the static map but they will never be accessed. Whether this was done by design or not, it is confusing. A way to change that is modify the migration and add a process plugin. Fortunately, these are similar to the d6 versions, so nothing radical.
In d7_field_formatter_settings.yml the field type is added as the first source to the static map. The new plugin, d7_field_type_defaults, is after the static map. When a formatter is not found in the static map the new plugin will do what the previous version of the static map did, that is return the formatter_type.
No interdiff as this is different approach that doesn't change processFieldFormatter().
Comment #8
jofitz CreditAttribution: jofitz at ComputerMinds commentedHere's an attempt at a review:
Re-using the logic from the d6 migration makes good sense to me.
Renaming the d6 plugin (now that there is a d7 equivalent) maintains a sensible naming convention, but will this renaming have repercussions?
Some of the "options/type" mappings have been removed (e.g. date_default, hidden) - is this intentional? Is there a way to continue handling these?haha, scrub that, I have now seen that they are handled in the plugin.In the d7 migration template you have retained the existing functionality of skip_on_empty if a value does not map, but the d6 version throws an exception in the plugin - should they both have the same functionality (this may be beyond the scope of this ticket).
Should the plugin name be changed from "field_type_defaults" to "d6_field_type_defaults" in setup() of d6/FieldTypeDefaultsTest.php:
$this->plugin = new FieldTypeDefaults([], 'field_type_defaults', []);
(If this is the case then I'm surprised this test still passes)
This should be "d7\FieldTypeDefaults"
Comment #9
joelpittetYes it will have repercussions existing migrations will not find the plugin:
exception 'Drupal\Component\Plugin\Exception\PluginNotFoundException' with message 'The "field_type_defaults" plugin does not exist.' in core/lib/Drupal/Component/Plugin/Discovery/DiscoveryTrait.php:52
I'm also surprised the test passes too, it really shouldn't.
Comment #10
joelpittetCould we add
field_type_defaults
as a plugin, leave the test alone(maybe ensure it works as a test). And leavefield_type_defaults
as a BC layer ford6_field_type_defaults
?Comment #11
joelpittetTrying my idea out, not sure how to ensure the test is doing what it should but let's see.
Comment #12
joelpittetShould the tests be also added to D6?
This seems related btw: #2831531: Improved the exception message in d6/FieldTypeDefaults
Comment #13
heddnSelf assigning to review this next week.
Comment #14
heddnThis seems to be dropped from the mapping. Ah, no. It isn't dropped.
But that demonstrates my next thoughts. It would be good to have tests for all of these in D6/D7 to make sure we don't introduce any regressions. A data provider pattern in the test would be a great method to do this.
Comment #15
mikeryanComment #17
hussainwebRerolling...
Comment #18
hussainwebComment #20
hussainwebI rebased against a very old copy of 8.4.x branch and when I caught up, a lot had changed and I had to dig through several issues to make sure I was not breaking anything. Here are the changes.
Comment #23
maxocub CreditAttribution: maxocub as a volunteer commentedPatch no longer applies, I'm working on a re-roll and tests.
Comment #24
maxocub CreditAttribution: maxocub as a volunteer commentedHere's the re-roll.
Comment #25
maxocub CreditAttribution: maxocub as a volunteer commentedThis is more complicated and messy than I thought.
In the d6_field_formatter_settings migration:
The first source of the "options/type" property is "type", the field type, so in the static map the first level of the array is keyed by field types.
But in the processFieldFormatter() method in FieldPluginBase:
The formatter map is first keyed by the field plugin id.
This work well when the field plugins have the same name as the field type, but it's not always the case. In core, we often have different field plugins for D6 & D7, so they can't have the same ID. Also, one field plugin could be used for different field types (number_integer, number_decimal & number_float). For all these reasons, assuming the field plugin IDs are the same as the field types is not very solid. I think we'll need to fix this.
Comment #26
quietone CreditAttribution: quietone as a volunteer commentedThat fits with my memory of this problem and you have explained it really well.
Comment #27
maxocub CreditAttribution: maxocub as a volunteer commentedThis is a first try, but I still need to check if all formatter types are covered. Let's see if the tests pass.
I moved all the static_map into the getFieldFormatterMap() method of the different field plugins, so if the tests pass, I think it should proves the the fix works.
Comment #29
maxocub CreditAttribution: maxocub as a volunteer commentedNeeds review.
Comment #30
heddnAssigning to myself to review later this week.
Comment #31
heddnHow do we test that all the field formatter maps are having an effect? Do we need to open up a lot of little issues to test all of them? Anyway, here's some thoughts. Really the first thing is the only thing I found. Except for the worry about how to test all of this.
We should add @deprecated and trigger_error here as per https://www.drupal.org/core/deprecation.
I don't think we want to move the class out of the d6 namespace if we want to retain BC. Err... actually, what we are retaining here is the plugin id. So that doesn't apply. Good thinking here. We still have the d6 namespaced plugin. And all classes would start using it if they extended things. But we keep the old plugin around in a new namespace. Perfect.
Comment #32
maxocub CreditAttribution: maxocub as a volunteer commentedI added the deprecation warning, along with a unit test and a change record.
About your first point regarding how to test all the field formatter maps, I think they are already covered by core/modules/field/tests/src/Kernel/Migrate/d7/MigrateFieldFormatterSettingsTest.php.
I think the fact that this test still passes after moving the static map to the different getFieldFormatterMap() methods proves that they work.
Comment #33
heddnAssuming this comes back green, all feedback is addressed. There's a change record. It's linked into the deprecation. Let's rock and roll.
Comment #34
maxocub CreditAttribution: maxocub commentedOups, this should not have been removed. This plugin is used by both D6 & D7 and these formatter types are in the D6 Email module.
This means that this is not tested. These formatter types are not present in the D6 fixture so we can't easily test them now.
I propose that we open one follow-up to check that *all* formatter types from Drupal 6 & 7 are mapped and tested. We should also open one for the widget types.
Comment #35
maxocub CreditAttribution: maxocub as a volunteer commentedFollow-ups created:
Comment #36
heddnCan we update the issue summary? I'm ok with the follow us. Let's make it clear though in this issue what we are doing. And what the follow ups will accomplish.
Comment #37
maxocub CreditAttribution: maxocub as a volunteer commentedIS updated.
Comment #38
maxocub CreditAttribution: maxocub as a volunteer commentedFurther IS improvements.
Comment #39
heddnThanks for such a nice IS update. One question, does it make sense to make d6 the same as d7 and move all of its static map changes into
getFieldFormatterMap()
as well? If we think we should, we can open that up as a follow-up or add it to the scope of the existing follow-ups.Comment #40
maxocub CreditAttribution: maxocub as a volunteer commentedIt would be great if we move the d6 static map to the field plugins and clean it up.
Just looking at the email field type we can see inconsistency.
From the d6_field_formatter_settings:
And from the Email field plugin:
The formatter types are not the same and the mappings are different.
But this is out of scope here
and I think we should add this to the scope of the follow-ups(Done).Comment #41
heddnIssue summary is updated. We've punted some things to follow-ups. A draft CR was added. I think we are good again.
Comment #43
catchtypo:s/of/or - fixed on commit.
This looks OK to backport to 8.5.x, so moving there and 'to be ported'.
Committed b89d3e4 and pushed to 8.6.x. Thanks!
Comment #44
webchickDiscussed on the core committer call; it seems advantageous to commit this to 8.5.x given we are doing a big push around how Migrate is ready for site builders.
Comment #46
heddnOf course #34 won't apply in 8.6. Can we post a patch against 8.5 here so it can sit in the rtbc queue? Tagging novice.
Comment #47
alexpottCommitted 594a0f6 and pushed to 8.5.x. Thanks!
Discussed with @catch and we felt this should be in 8.5.0.