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
To rename the module to "actions UI" migrations should be moved to system module because entity defined exactly there
Proposed resolution
\Drupal\action\Plugin\migrate\source\Action
should be moved to system or migrate_drupal because the action entity defined in system module and module will be renamed to provide UI just to configure actions
Remaining tasks
get agree on move (keep BC)
patch & commit
User interface changes
no
API changes
plugin and some classes will be moved
Data model changes
Release notes snippet
Comment | File | Size | Author |
---|---|---|---|
#80 | 3067299-80.patch | 8.07 KB | benjifisher |
#80 | interdiff-3067299-79-80.txt | 1.08 KB | benjifisher |
#79 | 3067299-79.patch | 9.34 KB | benjifisher |
#79 | diff-3067299-69-79.txt | 6.81 KB | benjifisher |
#69 | 3067299-69.patch | 13.13 KB | quietone |
Comments
Comment #3
iyyappan.govindStarting the initial patch for this issue.
Comment #5
andypostFix one of tests, the remaining ones checking
Comment #6
andypostFix bit more
Comment #9
andypostAnd one more down
Comment #10
andypostAnd remaining 2 down, I found that action module was not enabled in test so numbers were lower
It's still not clear the logic of test
Comment #12
andypostAdded CR https://www.drupal.org/node/3110401
Patch adds BC for migration class https://www.drupal.org/core/deprecation#how-plugin
Comment #13
andypostActually plugin just moved so using https://www.drupal.org/core/deprecation#how-class
Comment #14
alisonPatch applies cleanly on 8.9.x for me, and tests run fine (6 "legacy deprecation notices", but I'm sure y'all already see that -- I'm wayyyyy new at unit test things, so I'm sharing "everything" just in case).
(One hunk fails on 8.8.x, but that's irrelevant, right? -- it's the action_settings.yml hunk)
Is there anything else to do to "review" the patch, or just waiting for folks to express agreement/disagreement on the location things are getting moved to (system vs. migrate_drupal)?
Comment #15
iyyappan.govindComment #16
mikelutzThis looks good to me. We are free to move migration plugins around as long as we keep the ids. The only thing I'm not sure on is whether we can deprecate in 8.9 for removal in 10.
I don't believe we can deprecated in 8.9 for removal in D10. As I understand it, we are supposed to wait a few more weeks for the 9.1.x branch to open. So lets, do this, but put @todos in and/or file a follow-up against 9.1.x to actually add the deprecation.
Comment #18
andypostRe-roll for 9.1 and probably we still needs deprecation here to detect usage if anything inherits from old namespace
Comment #19
andypostLooks state in system module does not need update
Comment #20
quietone CreditAttribution: quietone as a volunteer commented@andypost, thanks for the updated patch.
In this case that is true. The system.migrate_drupal.yml already has entries
system: system
declared in 'finished' for both 6 and 7. At the time that file was created all the migrations with a destination of 'system' were considered finished. The upcoming move of the action migrations wasn't taken into consideration. And if it had been the action state file entries should have been usingsystem system
.I agree with mikelutz in #16 that this looks good. I also updated the CR, adding more information and trying to improve the readability.
Remove 'the'. It doesn't read correctly and isn't the standard format from the deprecation policy.
Comment #21
mrinalini9 CreditAttribution: mrinalini9 at Srijan | A Material+ Company for Drupal India Association commentedComment #22
mrinalini9 CreditAttribution: mrinalini9 at Srijan | A Material+ Company for Drupal India Association commentedUpdated patch as per the changes suggested in #20, please review.
Comment #23
mrinalini9 CreditAttribution: mrinalini9 at Srijan | A Material+ Company for Drupal India Association commentedComment #24
mrinalini9 CreditAttribution: mrinalini9 at Srijan | A Material+ Company for Drupal India Association commentedPlease ignore patch #22, here is the updated patch with the changes mentioned in #20, please review.
Comment #25
quietone CreditAttribution: quietone as a volunteer commented@mrinalini9, Wrong patch perhaps?
This file and others are not to be deleted they are to be moved to the system module.
Comment #26
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedThis patch might fix comment #20.
Comment #28
nitesh624Comment #29
longwaveRandom fail, retesting.
Comment #30
andypostRe #25
We should remove this migration as we can after #3039240: Create a way to declare a plugin as deprecated so for now yep it just should be moved
Comment #31
quietone CreditAttribution: quietone as a volunteer commentedI agree, the migration destination changed to null in #3022401: Remove useless config action.settings.recursion_limit so it isn't doing anything but we need a follow up for removing the migration yml.
Comment #32
andypost@quietone maybe we can use the issue and this migration as example? #3039240: Create a way to declare a plugin as deprecated
Comment #33
quietone CreditAttribution: quietone as a volunteer commented@andypost, if you mean that we use the deprecation of action_settings.yml as the example in #3039240: Create a way to declare a plugin as deprecated, then yes, I agree.
Comment #35
andypostFiled follow-up #3209220: Deprecate and remove action settings migration and here's re-roll for 9.2.x
Comment #37
quietone CreditAttribution: quietone as a volunteer commentedThis needs a reroll and it looks suitable for a novice.
Comment #38
iyyappan.govindRe-rolled the patch to 9.2.x and 9.3.x
Comment #39
iyyappan.govindComment #40
andypostno reason for 9.2 patch
Comment #41
iyyappan.govindandypost I am sorry, I didn't see your message.
Comment #42
longwaveThanks for rerolling.
All references to drupal:9.2.0 need updating to drupal:9.3.0 now. As @andypost mentions this can only be committed to 9.3.x.
Comment #43
yogeshmpawarAddressed #42
Comment #45
iyyappan.govindTrying to fix failed test cases;
Comment #47
longwavePlease post interdiffs, they make it much easier to review your changes.
Comment #48
iyyappan.govindChanged entity count
Comment #49
longwaveThese changes seem to have been made in #45 but to me are out of scope.
Comment #50
iyyappan.govindFixed the comment issue mentioned on #49.
Comment #51
longwaveOne thing I guess I don't understand is why does this change, if we are only moving things around? The number of entities should stay the same?
Comment #52
longwaveIn fact @andypost answered this in #10, action module was not previously enabled for these tests so the entities were not migrated before.
Everything else looks good so this is ready to go as far as I can see.
Comment #54
longwaveHit by #3208791: [random test failure] Random fail in LayoutBuilderDisableInteractionsTest, back to RTBC
Comment #56
andypostre-roll for 9.4.x
Comment #57
quietone CreditAttribution: quietone as a volunteer commented@andypost, thanks for the update.
When I looked at the interdiff I realized that this is missing a test of the deprecation. This patch adds that and adds a missing space character in the class @trigger_error message.
Comment #59
quietone CreditAttribution: quietone at PreviousNext commentedNeed to do a bit of renaming.
Comment #60
andypostThank you,, missed this changes
Comment #62
andypostComment #64
quietone CreditAttribution: quietone at PreviousNext commentedRe running tests.
Comment #65
andypostback to rtbc
Comment #66
quietone CreditAttribution: quietone at PreviousNext commentedReroll due to #3266443: Rename StateFileExists to StateFileExistsTest
Comment #67
quietone CreditAttribution: quietone at PreviousNext commentedShould br NR
Comment #69
quietone CreditAttribution: quietone at PreviousNext commentedAnd update the new migrate tests in aggregator.
Comment #70
andypostThank you
Comment #72
andregp CreditAttribution: andregp at CI&T commentedI applied the patch, ran the failed test locally, and it passed without errors. So I believe that was a random fail and queued a retest to confirm.
Comment #73
andregp CreditAttribution: andregp at CI&T commentedThe error on D10 was indeed random, the second test passed so I'm restoring the previous status.
Comment #75
quietone CreditAttribution: quietone at PreviousNext commentedFailure is Drupal\Tests\quickedit\FunctionalJavascript\QuickEditFileTest::testRemove
Resetting the RTBC
Comment #76
danflanagan8I'm going to grab this and try the re-roll, per an open request by @benjifisher in Slack.
Comment #77
danflanagan8nm, @benjifisher took it! Unassigning.
Comment #78
danflanagan8I'm pretty sure this will require a follow-up in the contrib Aggregator module. I have filed the issue and postponed it on this one: #3274042: Update migrate_drupal_ui tests in Aggregator to account for changes to action
Comment #79
benjifisherHere is a reroll. I applied the patch from #69 to an earlier commit on the 10.0.x branch (4d32bbccb8, if anyone cares) and merged with the current HEAD of 10.0.x. There were only a few merge conflicts, all from #3264120: Remove aggregator module and our dependency on Laminas Feed:
@danflanagan8, thanks for creating #3274042: Update migrate_drupal_ui tests in Aggregator to account for changes to action. That should be a very easy issue, since (1) and (2) only change a couple of entity counts in the tests.
The other file that confused me is core/modules/action/src/Plugin/migrate/source/Action.php: an empty class that is only there to trigger a deprecation error. We do not want that class in the 10.0.x branch.
In fact, the patch in #69 should never have been applied to the 10.0.x branch.
I am attaching a reroll of the patch in #69 and a diff (not an interdiff). I will ask the testbot to re-test #69 against 9.4.x, but I will not test the new patch at all. I am pretty sure that there are some deprecation tests that have to be removed.
Comment #80
benjifisherJust one. Patch and interdiff attached.
Comment #81
benjifisherI tried (twice) to re-test #69 against 9.4.x, but it seems to be queued for 10.0.x.
I am not too worried about the extra load on the testbot, since I already know the patch will not apply.
Comment #83
danflanagan8That fail was some random quickedit thing.
Due to some miscommunication I did most of the re-roll myself until I found out @benjifisher was on the case. Therefore feel like I'm in a nice position to RTBC. Looks good to me. RTBC!
Just to try to keep things clear:
9.4.x => #69
10.0.x => #80
Both RTBC.
Comment #86
catchThis all looks great and will make it easier to remove the actions module itself from core if/when we go ahead with that. Migrations will only be in system module until core migrations are also moved to contrib (in Drupal 11 or so), which means it's not contributing to long-term system module bloat either.
Committed/pushed to 10.0.x and the respective patch to 9.4.x, thanks!
Comment #87
andypostThank you! The only blocker left is #2811663: Rename Action module to Actions UI in the UI and in comments