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
Following the proposed solution for #2842222: D7 Plain text fields incorrectly migrated to D8 as Text (formatted) as outlined in #107, we need a plugin to process the field type other than the current static map.
Proposed resolution
Write a process plugin called process_field which will take two configuration: the source field type and the name of a method to be called.
The plugin will then instantiate the appropriate Field plugin based on the given field type and call the given method on the instantiated Field plugin, which will return the processed destination field type.
Remaining tasks
Write a patch, Review
User interface changes
None.
API changes
None.
Data model changes
None.
Comment | File | Size | Author |
---|---|---|---|
#29 | interdiff-2893061-27-29.txt | 4.13 KB | maxocub |
#29 | 2893061-29.patch | 11.22 KB | maxocub |
#27 | interdiff-2893061-24-27.txt | 4.39 KB | maxocub |
#27 | 2893061-27.patch | 11.26 KB | maxocub |
#24 | interdiff-2893061-23-24.txt | 3.02 KB | maxocub |
Comments
Comment #2
maxocub CreditAttribution: maxocub commentedHere's a first patch.
Comment #3
maxocub CreditAttribution: maxocub commentedUpdated the plugin doc to make it's usage more general.
Updated the d7_field migration to make use of the new process plugin.
Postponed on the Field plugin issues:
Comment #5
maxocub CreditAttribution: maxocub commentedBumping to critical since this is a hard blocker for #2842222: D7 Plain text fields incorrectly migrated to D8 as Text (formatted).
The issues this was postponed on can be dealt in a follow-up (#2901851: Replace the static map in the d7_field migration by field plugins), so I put it back to Needs review and added a @todo.
Comment #6
jofitz CreditAttribution: jofitz at ComputerMinds commentedApplied the patch to my local build and stepped through MigrateFieldTest.php as the prime example of how this should work. The new plugin runs along the established lines of the "field_type" plugin and works as expected.
The changes added in #5 makes sense to speed up the acceptance of this patch.
Comment #7
catchThere's no explicit test coverage for this, shouldn't there be some?
Comment #8
maxocub CreditAttribution: maxocub commentedYes indeed there should be, I'll try to add some.
Comment #9
maxocub CreditAttribution: maxocub commentedHere's a Unit test.
Comment #11
maxocub CreditAttribution: maxocub commentedI corrected the coding standard errors and I added
@group legacy
to the test so it won't fail due to the deprecation warning for MigrateCckFieldPluginManagerInterface.I'm not sure if it's the best way to make the test pass.
I could also just remove the cck code from the plugin now, since it's deprecated anyway, or we can deal with cleaning this up in #2897593: Cleanup further references to CCK.
What do you think?
Comment #12
catchAll depends on what's going to rely on the deprecated code, which I'm not entirely sure about at this point tbh. I'd lean towards leaving it in unless it's already effectively dead code.
Just committed #2901851: Replace the static map in the d7_field migration by field plugins by the way but afraid this means another re-roll here.
Comment #13
jofitz CreditAttribution: jofitz at ComputerMinds commentedAs mentioned in #12, a re-roll was required.
Comment #15
jofitz CreditAttribution: jofitz at ComputerMinds commentedTest failures are unrelated to this issue, they occur even when the patch is not applied (which is concerning...)
Comment #16
maxocub CreditAttribution: maxocub commented@catch, re #12:
In core, it is dead code, but maybe some contrib/custom module have created there own cckfield plugins and have not yet updated them to field plugins. I guess we better leave the cck code for now and deal with it in #2897593: Cleanup further references to CCK.
Comment #17
xjm@cilefen, @effulgentsia, @Cottser and I discussed this and agreed it makes sense to handle it as a critical as a blocker for #2842222: D7 Plain text fields incorrectly migrated to D8 as Text (formatted). While the title doesn't really describe a critical issue on its own, and we don't typically hard-block criticals on API additions, it's helpful here because (as @catch mentioned) we lost track of an earlier issue in the chain and the rest have gone quickly once we were able to promote them for visibility. And, as @effulgentsia mentioned, there isn't really a good solution for this currently, so there's not some hotfix we should pursue instead.
Thanks for the ongoing work on this chain of issues!
Comment #18
phenaproximaThis is an excellent patch. First-class work all around! It wouldn't be me if I didn't raise a few questions, though :)
I think we should probably return NULL instead. So that, say, one could skip_on_empty. Either that, or throw an exception? If no appropriate field plugin is found, I think it's safer to consider that an uncertain condition that requires developer intervention.
Nit: Should match the constructor parameter.
Let's change $value[0] to reset($value), just in case $value is associative.
Can we abstract this a bit as a helper method that accepts the plugin manager to try, rather than repeat the code entirely?
Interesting, I've never seen this test group before in core. Is it a new thing?
I think we should refactor this test class to use the data provider pattern, which would make it easier for us to test new methods, new combinations and types of arguments, etc.
I would prefer if we used Prophecy for all this, but I'm not married to it.
Also, for clarity, can $this->fieldInstance be renamed to $this->fieldPlugin?
Comment #19
catchOn #5 it allows the test to pass without warnings despite it using deprecated code. https://www.drupal.org/core/deprecation
Comment #20
catchMoving back to 8.4.x since it's eligible.
Comment #21
maxocub CreditAttribution: maxocub commentedThanks for the review @phenaproxima!
1. It now returns NULL.
2. Done.
3. Done.
4. Done.
5. See #19.
6. Added a data provider.
7. Changed for Prophecy.
Comment #22
phenaproximaAll the karma unto @maxocub. You are a rock star.
A few more things jumped out at me, but they should all be reasonably easy fixes. They basically come down to documentation.
Missing @return documentation.
Is the method_exists() call necessary? Won't is_callable() do essentially the same thing?
Ideally, $migrate_exception would be the full or partial text of the exception message, and we would assert that. ($this->setExpectedExceptionMessage() or something)
Can you add some comments here to briefly explain each scenario we're running, what we are doing in each, and what we expect to happen? I <3 the data provider pattern, but it can sometimes obfuscate our intentions.
Comment #23
maxocub CreditAttribution: maxocub commentedFast review = fast new patch.
1. Done.
2. You're right, it's superfluous.
3. setExpectedExceptionMessage() doesn't seem to exists on this test class, but I added the expected message in the setExpectedException() call.
4. Done.
Comment #24
maxocub CreditAttribution: maxocub commentedChanged the $migrate_exception variable type from bool to string, based on a discussion with phenaproxima on irc.
Also added some @params on the test method.
Comment #25
phenaproximaThis looks beautiful and should pass Drupal CI. RTBC once it does.
Comment #26
heddnA couple questions/comments.
In what cases could this be an array? Are we assuming the first array value is the field_type? That seems like magic and bad.
I also don't see a test for this scenario, so that maybe would clear up what we're trying to do here.
It would be helpful if the keys on this were the variable names being passed into the test function.
Comment #27
maxocub CreditAttribution: maxocub commented1. @heddn and I decided on irc that we should not expect an array, only a string representing the field type. If it's not a string, we throw an exception. I added test data for that scenario.
2. Done.
Comment #28
heddnI think this should be is_scalar. See https://3v4l.org/lVPqC
This should be $value instead of $field_type, no?
Comment #29
maxocub CreditAttribution: maxocub commented1. I don't think so, a field type cannot be an integer, and is_scalar return true for an integer.
2. Changed $field_type to $value.
Comment #30
heddnAll feedback addressed. Back to RTBC if tests pass.
Comment #34
larowlanCommitted as bd2227e and pushed to 8.5.x.
Cherry-picked as 63bd90d and pushed to 8.4.x
Great work!
Comment #35
larowlanOh - can we get a change record here?
Comment #36
maxocub CreditAttribution: maxocub commented@larowlan: Sure, I'll add one. Thanks for commiting this!
Comment #37
maxocub CreditAttribution: maxocub commentedI created a change record. I left it in draft even though the issue is already fixed so someone can review it, correct it if needed, and publish it.
Comment #38
larowlanpublished that
Comment #39
maxocub CreditAttribution: maxocub for Acquia commentedComment #40
maxocub CreditAttribution: maxocub as a volunteer and for Acquia commentedComment #41
xjmI don't think we need to mention this issue specifically, just the one that it unblocked which is the big win. Thanks all!