Problem/Motivation
The 'date' field plugin for Drupal 6 was replaced with a 'datetime' field plugin that works for both Drupal 6 and 7 migrations. Despite deprecating the original date plugin, the plugin manager still creates an instance of it when requesting a plugin for a D6 date field because it is found first in the plugin list and the annotations say it applies. Removing the type_map from the annotations partially solves this, but due to the fact that the field name and plugin id match, it is still selected to manage D6 date fields even though it shouldn't be anymore.
Due to this, the deprecation error triggered by this plugin was ignored globally, creating technical debt that needs to be resolved for Drupal 9.
Additionally, the same system makes it difficult or impossible for contrib plugin to override a core MigrateField plugin for a specific field. Because the first discovered applicable plugin is used, there is no way to guarantee a contrib plugin will actually override the core plugin.
Proposed resolution
Adding a weight field to the plugin annotation will allow plugins to declare the order in which they should be used for field migrations. Deprecated plugins can be set to a higher weight, and contrib overrides can be set to a lower weight. Applying a sort first by weight, then by plugin ID will assure a single deterministic, testable, and repeatable plugin will be used for any given field and core version.
Remaining tasks
Add a weight field to the MigrateField annotation
Sort the plugins by weight and plugin ID in MigrateFieldPluginManager
Add/refactor/adjust tests
Remove the date plugin deprecation from the global ignore list.
User interface changes
None
API changes
MigrateField plugins now have a weight field in their annotation that determines which one is used for a given field and Drupal core version.
Data model changes
None.
Comments
Comment #2
mikelutzComment #3
mikelutzNot sure I'm 100% happy with this one, but let's see what the tests say.
Comment #4
mikelutzComment #5
mikelutzWell, the patch itself needs a little polish, but I guess I need some feedback on the approach. It passes tests with a small patch, and I don't think it actually breaks anything.
Comment #6
mikelutzAnother thought, might add too much extra stuff, but at least it's not a one-off line of code to remove that plugin.
Comment #8
mikelutzugh, I had some cross-contamination from another issue I was working on.
Comment #9
mikelutzComment #10
mikelutzFrom discussion with @phenaproxima, I guess we don't actually have to change the annotation class to add a flag. I don't love undefined/undocumented annotations as signalling flags, so I'm open to other ideas, but we need to somehow exclude that plugin from being called.
Comment #11
mikelutzComment #12
maxocub CreditAttribution: maxocub commentedI like this. I think we should also open a follow-up to add the deprecated flag to other deprecated filed plugins so they are not use by accident.
After a quick grep, I found that this should also be done in the
Drupal\user\Plugin\migrate\User
class.Super-nit: this extra line is not useful and only add noise to the patch.
Comment #13
mikelutzSo I did add the line to User, but here's the deal..
Both User and D7 comment wrap that plugin creation in a ->hasDefinition() check. So even if a plugin declares itself eligible to process a field in the type_map, it's still not getting used in either case unless it's plugin_id matches the field name. All changing it from createInstance to getPluginIdFromFieldType does is eliminates that deprecated plugin now. I did it on D7Comment because I had to to pass tests, but we have no date field on User in the fixture, so that one didn't trip. Both will still fail to find a field plugin if the plugin Id and field name don't match.
Both of these really need to use the node/taxonomy deriver pattern where the getPluginIdFromFieldType() is wrapped in a try/catch(PluginNotFoundException), or better yet have a pluginExistsForFieldType() method in MigrateFieldPluginManagerInterface so we don't use exceptions as a signal.
The rabbit hole of bugginess quickly took trying to fix anything in those two out of scope for this issue. User is being refactored in #2951550: Make service for field discovery for use in migrate entity derivers and D7Comment has an issue at #3005718: D7 comment migration does not properly migrate fields by comment bundle., so the initial patch was trying to make the minimum fixes I needed to do to solve this issue.
I removed the unneeded extra line.
Comment #14
maxocub CreditAttribution: maxocub commented@mikelutz: Thanks for the explanation. I agree D7Comment and User should be refactored to be more like the node and taxonomy term derivers.
RTBC provided the tests are green.
Comment #15
alexpottThis is not how we deprecate plugins. We've learnt the hard way that plugins should be deprecated via the constructor and not in the main part of the class. See https://www.drupal.org/core/deprecation#how-plugin
Then there'll be no random keys in the annotation and no unintended API breaks such as:
Comment #16
mikelutzI'm happy to move the @trigger_error into the constructor, but we still need a way to make sure it isn't returned from getPluginIdFromFieldType(). I'm open to suggestions.
Comment #17
mikelutzHere's a thought, lets let those plugins declare a weight. This allows contrib to override a core plugin for a field and lets us lower the weight on deprecated plugins.
Comment #18
alexpottThis looks like a good flexible solution. +1 - this is why skipping deprecations is a bad idea means we don't solve these things at the right time.
Let's do
uasort(definitions, ['Drupal\Component\Utility\SortArray', 'sortByWeightElement']);
But I also wonder if we should do a stable sort here - ie. sort by weight and plugin_id.
Can remove this change
Comment #19
mikelutzQuick sanity check, and then I'll work on CR, tests, stable sort, ect...
Comment #20
mikelutzOkay, here's a fleshed out version. I used a stable sort and moved it to an overridden getDefinitions method so we can cache the sort. I completely reworked the tests around MigrateFieldPluginManager. The updated tests caught a typo in the d6 link plugin definition (it's type_map referenced the d7 'link_field' field type instead of the d6 'link' field_type) so I fixed that here as well.
Comment #21
mikelutzAdd CR, change IS and title to reflect what we are doing.
Comment #22
mikelutzComment #23
heddnNit: need full path to class.
Except for the 99999 deprecated, can we remove the rest of these or at least some of these to test what happens when we don't explicitly define the weight?
Comment #24
mikelutzAfter a discussion with @phenaproxima, I moved the sort into findDefinitions, which lets me inject the sort before the cache while still keeping more of the default discovery code path.
I added the namespaces to the test docs.
I can't remove the weight on that unit test, as it is simulating the result of the plugin discovery which already has the default value set in the annotation class applied. I did add a kernel test to check that the weight is set, and set to zero for all non-deprecated core plugins.
Comment #25
heddnValid thoughts. Here's a few more to consider.
Why did this inheritdoc go away?
Is this needed any more?
Comment #26
mikelutz1 - I added documentation to the method to indicate how it was selecting which plugin to return.
2 - no, I'm still overriding the constructor to insert a mock discovery object.
Comment #27
mikelutzThis adds an update function to clear the cache.
Comment #28
mikelutzQuick fix on a bad test.
Comment #29
mikelutzAnd the right diff...
Comment #30
heddnSo, is there any way to test that the weight annotation wasn't there before the cache clear? It would seem the answer is no.
Comment #31
mikelutzNo, the initial state of the cache would have to be part of the test configuration and set up by definition. For the purposes of that test though, I don't care about the previous state of the cache, all I care is that after the update hook runs, whatever the previous cache was, I end up with a useable plugin list. Which is why I didn't worry about populating it with anything other than an empty array.
Comment #32
phenaproximaLookin' really good. I think this is very logical, and it's nice to see the deprecation moved into the constructor and out of the "global" deprecations. My feedback is mostly minor.
Nit: This should have a trailing comma.
Same here.
Let's replace this with an empty post-update hook, which will force a general cache clear and does not need a test.
Can we say "Drupal core" instead of "core Drupal"?
s/who/which
'id' should be ID. And can we add a @see annotation to this method, referring to the annotation class?
We can remove the |null from here. "(optional)" covers it.
field_type should be "field type".
'id' --> ID
Can we use assertSame() when comparing strings?
We should probably replace this with a dataProvider pattern and $this->setExpectedException(), since it's more explicit and clear than $this->fail() in a try-catch.
This assertion is not needed (and therefore, neither is the $plugin variable).
in_array() should be called with TRUE as a third argument.
Should be assertSame().
We can remove this once we replace the update function with an empty post-update.
Nice :)
This should use $this->setExpectedException() (the data provider can pass the expected exception class and message, if an exception is expected).
Comment #33
mikelutzI addressed all feedback, I believe.
Comment #34
phenaproximaLooks great. Two tiny nits:
Minor spelling error: this should be nonExistent.
This should be assertSame().
Comment #35
mikelutzThanks! There ya go.
Comment #36
mikelutzFix for the PHP 5.5 test
Comment #37
mikelutzIS formatting fixes.
Comment #38
phenaproximaRTBC on green for all backends. Thanks!
Comment #39
alexpottHow come this change is necessary? And what is the impact on existing migrations due to this?
code style...
Comment #40
mikelutz39.1 - I expanded the tests around field plugin discovery in this issue, and one of the tests I added checked that drupal 6 plugins wouldn't be created for drupal 7 only field names and vice versa. In Drupal 6 the CCK link field was called 'link', and in Drupal 7 it was called 'link_field'. The expanded test checks that there is no 'link_field' plugin for D6 nor a 'link' plugin for D7, among others. When I ran it, it failed by creating a D6 plugin for 'link_field', which it shouldn't. I didn't want to reduce the test coverage just to avoid fixing the typo. The plugin worked for link fields anyway because of the plugin id. There is no impact on existing migrations since there was no such thing as 'link_field' in Drupal 6, and the plugin is already used for D6 'link' fields.
.2 Fixed.
Comment #41
mikelutzComment #43
phenaproximaLooks like it needs a reroll...
Comment #44
mikelutzRerolled against another commit removing things from the deprecation list.
Comment #45
phenaproximaAll feedback seems to be addressed.
Comment #46
quietone CreditAttribution: quietone as a volunteer commentedA question,
Why is a subset of the field plugins tested? Why these particular ones and not others? Notably all the reference fields are absent. Should all of them be tested? If this warrants a comment in code please, please sort the $modules list in the test. Otherwise, no worries
Comment #47
mikelutzWe could test more, but we aren't really testing the plugins here, we are testing the logic of the plugin manager. I only added to what was there and changed the asserts to not use the deprecated assertIdentical. The ones tested are a mix across drupal versions, ones found by typemap, and ones found by plugin Id. I added ones around the new deprecation weights. To me it seems enough to prove the logic of the manager, and I assume the individual plugins are themselves tested elsewhere.
Comment #48
quietone CreditAttribution: quietone as a volunteer commentedThanks, just wanted to be sure I wasn't missing something. I spotted that the reference ones weren't they and wondered, so I asked without giving it much thought. So, no change needed. 'Tis great as it is.
Comment #49
catchNice approach. I don't think we can backport this to 8.6.x, but if you strongly disagree please re-open.
Committed 6b14845 and pushed to 8.7.x. Thanks!
Comment #51
mikelutzI published the associated change record. I never expected this to be portable to 8.6. It does technically contain a bug fix in that it prevents the deprecated plugin from being called, however because of the BC shim left in place, this 'bug' has no negative side effects. The patch contains new functionality and should just be included in 8.7 Thanks!