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

CommentFileSizeAuthor
#80 3067299-80.patch8.07 KBbenjifisher
#80 interdiff-3067299-79-80.txt1.08 KBbenjifisher
#79 3067299-79.patch9.34 KBbenjifisher
#79 diff-3067299-69-79.txt6.81 KBbenjifisher
#69 3067299-69.patch13.13 KBquietone
#69 interdiff-66-69.txt1.28 KBquietone
#66 3067299-66.patch11.86 KBquietone
#66 diff.-59-66.txt1.38 KBquietone
#59 3067299-59.patch11.77 KBquietone
#59 diff-57-59.txt754 bytesquietone
#57 3067299-57.patch11.75 KBquietone
#57 diff-56-57.txt2.13 KBquietone
#56 3067299-56.patch10.49 KBandypost
#56 interdiff.txt1.25 KBandypost
#50 interdiff-48-50.txt1 KBiyyappan.govind
#50 3067299-50.patch11.3 KBiyyappan.govind
#48 interdiff-45-48.txt891 bytesiyyappan.govind
#48 3067299-48.patch11.77 KBiyyappan.govind
#45 3067299-45.patch10.57 KBiyyappan.govind
#43 interdiff-3067299-38-43.txt1.25 KByogeshmpawar
#43 3067299-43.patch10.1 KByogeshmpawar
#38 3067299-38-9.3.x.patch10.1 KBiyyappan.govind
#38 3067299-38-9.2.x.patch11.3 KBiyyappan.govind
#35 3067299-35.patch10.03 KBandypost
#35 interdiff.txt1.62 KBandypost
#26 interdiff_18-26.txt610 bytesravi.shankar
#26 3067299-26.patch11.09 KBravi.shankar
#24 interdiff-3067299-18-24.txt15.29 KBmrinalini9
#24 3067299-24.patch17.72 KBmrinalini9
#22 interdiff-3067299-18-22.txt15.29 KBmrinalini9
#22 3067299-22.patch17.72 KBmrinalini9
#18 3067299-18.patch9.99 KBandypost
#18 interdiff.txt1.17 KBandypost
#13 3067299-13.patch9.99 KBandypost
#13 interdiff.txt1.4 KBandypost
#12 3067299-12.patch10.41 KBandypost
#12 interdiff.txt1.35 KBandypost
#10 3067299-10.patch7.72 KBandypost
#10 interdiff.txt1.2 KBandypost
#9 3067299-9.patch6.52 KBandypost
#9 interdiff.txt872 bytesandypost
#6 3067299-6.patch5.97 KBandypost
#6 interdiff.txt407 bytesandypost
#5 3067299-5.patch5.69 KBandypost
#5 interdiff.txt605 bytesandypost
#3 3067299-03.patch5.46 KBiyyappan.govind
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

andypost created an issue. See original summary.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

iyyappan.govind’s picture

Status: Active » Needs review
FileSize
5.46 KB

Starting the initial patch for this issue.

Status: Needs review » Needs work

The last submitted patch, 3: 3067299-03.patch, failed testing. View results

andypost’s picture

Assigned: Unassigned » andypost
Status: Needs work » Needs review
FileSize
605 bytes
5.69 KB

Fix one of tests, the remaining ones checking

andypost’s picture

Assigned: andypost » Unassigned
FileSize
407 bytes
5.97 KB

Fix bit more

The last submitted patch, 5: 3067299-5.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 6: 3067299-6.patch, failed testing. View results

andypost’s picture

Status: Needs work » Needs review
FileSize
872 bytes
6.52 KB

And one more down

andypost’s picture

And 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

The last submitted patch, 9: 3067299-9.patch, failed testing. View results

andypost’s picture

andypost’s picture

FileSize
1.4 KB
9.99 KB

Actually plugin just moved so using https://www.drupal.org/core/deprecation#how-class

alison’s picture

Patch 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)?

iyyappan.govind’s picture

mikelutz’s picture

Status: Needs review » Needs work

This 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.

+++ b/core/modules/action/src/Plugin/migrate/source/Action.php
@@ -2,68 +2,18 @@
+@trigger_error('The' . __NAMESPACE__ . '\Action is deprecated in drupal:8.9.0 and is removed from drupal:10.0.0. Use \Drupal\system\Plugin\migrate\source\Action instead. See https://www.drupal.org/node/3110401', E_USER_DEPRECATED);
...
+ * @deprecated in drupal:8.9.0 and is removed from drupal:10.0.0. Use the
+ *   \Drupal\system\Plugin\migrate\source\Action instead.
+ *
+ * @see https://www.drupal.org/node/3110401

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.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

andypost’s picture

Status: Needs work » Needs review
FileSize
1.17 KB
9.99 KB

Re-roll for 9.1 and probably we still needs deprecation here to detect usage if anything inherits from old namespace

andypost’s picture

--- a/core/modules/action/migrations/state/action.migrate_drupal.yml
+++ /dev/null
@@ -1,5 +0,0 @@
-finished:
-  6:
-    system: action
-  7:
-    system: action

Looks state in system module does not need update

quietone’s picture

Status: Needs review » Needs work

@andypost, thanks for the updated patch.

In this case that is true. The system.migrate_drupal.yml already has entries system: systemdeclared 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 using system 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.

+++ b/core/modules/action/src/Plugin/migrate/source/Action.php
@@ -2,68 +2,18 @@
+ * @deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. Use the

Remove 'the'. It doesn't read correctly and isn't the standard format from the deprecation policy.

mrinalini9’s picture

Assigned: Unassigned » mrinalini9
mrinalini9’s picture

Assigned: mrinalini9 » Unassigned
Status: Needs work » Needs review
FileSize
17.72 KB
15.29 KB

Updated patch as per the changes suggested in #20, please review.

mrinalini9’s picture

Assigned: Unassigned » mrinalini9
Status: Needs review » Needs work
mrinalini9’s picture

Assigned: mrinalini9 » Unassigned
Status: Needs work » Needs review
FileSize
17.72 KB
15.29 KB

Please ignore patch #22, here is the updated patch with the changes mentioned in #20, please review.

quietone’s picture

Status: Needs review » Needs work

@mrinalini9, Wrong patch perhaps?

diff --git a/core/modules/action/migrations/action_settings.yml b/core/modules/action/migrations/action_settings.yml
deleted file mode 100644

This file and others are not to be deleted they are to be moved to the system module.

ravi.shankar’s picture

Status: Needs work » Needs review
FileSize
11.09 KB
610 bytes

This patch might fix comment #20.

Status: Needs review » Needs work

The last submitted patch, 26: 3067299-26.patch, failed testing. View results

nitesh624’s picture

Assigned: Unassigned » nitesh624
longwave’s picture

Status: Needs work » Needs review

Random fail, retesting.

andypost’s picture

Re #25

diff --git a/core/modules/action/migrations/action_settings.yml b/core/modules/system/migrations/action_settings.yml
similarity index 92%

rename from core/modules/action/migrations/action_settings.yml
rename to core/modules/system/migrations/action_settings.yml

+++ b/core/modules/system/migrations/action_settings.yml
@@ -17,4 +17,4 @@ process:
 destination:
   plugin: config
   config_name: null
-  destination_module: action
+  destination_module: system

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

quietone’s picture

Assigned: nitesh624 » Unassigned
Status: Needs review » Needs work
Issue tags: +Needs followup

I 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.

andypost’s picture

@quietone maybe we can use the issue and this migration as example? #3039240: Create a way to declare a plugin as deprecated

quietone’s picture

@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.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

andypost’s picture

Status: Needs work » Needs review
Issue tags: -Needs followup
FileSize
1.62 KB
10.03 KB

Filed follow-up #3209220: Deprecate and remove action settings migration and here's re-roll for 9.2.x

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

quietone’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll, +Novice

This needs a reroll and it looks suitable for a novice.

iyyappan.govind’s picture

iyyappan.govind’s picture

Status: Needs work » Needs review
andypost’s picture

no reason for 9.2 patch

iyyappan.govind’s picture

andypost I am sorry, I didn't see your message.

longwave’s picture

Status: Needs review » Needs work
Issue tags: -Needs reroll

Thanks for rerolling.

+++ b/core/modules/action/src/Plugin/migrate/source/Action.php
@@ -2,8 +2,9 @@
+@trigger_error('The' . __NAMESPACE__ . '\Action is deprecated in drupal:9.2.0 and is removed from drupal:10.0.0. Use \Drupal\system\Plugin\migrate\source\Action instead. See https://www.drupal.org/node/3110401', E_USER_DEPRECATED);

@@ -13,63 +14,11 @@
+ * @deprecated in drupal:9.2.0 and is removed from drupal:10.0.0. Use

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.

yogeshmpawar’s picture

Status: Needs work » Needs review
FileSize
10.1 KB
1.25 KB

Addressed #42

Status: Needs review » Needs work

The last submitted patch, 43: 3067299-43.patch, failed testing. View results

iyyappan.govind’s picture

Status: Needs work » Needs review
FileSize
10.57 KB

Trying to fix failed test cases;

Status: Needs review » Needs work

The last submitted patch, 45: 3067299-45.patch, failed testing. View results

longwave’s picture

Please post interdiffs, they make it much easier to review your changes.

iyyappan.govind’s picture

Status: Needs work » Needs review
FileSize
11.77 KB
891 bytes

Changed entity count

longwave’s picture

+++ b/core/modules/system/tests/src/Kernel/Migrate/d6/MigrateActionsTest.php
@@ -23,7 +23,7 @@ protected function setUp(): void {
-   * Tests Drupal 6 action migration to Drupal 8.
+   * Test Drupal 6 action migration to Drupal 8.

+++ b/core/modules/system/tests/src/Kernel/Migrate/d7/MigrateActionsTest.php
@@ -23,7 +23,7 @@ protected function setUp(): void {
-   * Tests Drupal 7 action migration to Drupal 8.
+   * Test Drupal 7 action migration to Drupal 8.

These changes seem to have been made in #45 but to me are out of scope.

iyyappan.govind’s picture

Fixed the comment issue mentioned on #49.

longwave’s picture

+++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d6/Upgrade6Test.php
@@ -95,7 +95,7 @@ protected function getEntityCounts() {
-      'action' => 27,
+      'action' => 33,

+++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d7/Upgrade7Test.php
@@ -97,7 +97,7 @@ protected function getEntityCounts() {
-      'action' => 21,
+      'action' => 27,

One 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?

longwave’s picture

Status: Needs review » Reviewed & tested by the community

In 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.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 50: 3067299-50.patch, failed testing. View results

longwave’s picture

Status: Needs work » Reviewed & tested by the community

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

andypost’s picture

quietone’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: -Novice
FileSize
2.13 KB
11.75 KB

@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.

Status: Needs review » Needs work

The last submitted patch, 57: 3067299-57.patch, failed testing. View results

quietone’s picture

Need to do a bit of renaming.

andypost’s picture

Status: Needs review » Reviewed & tested by the community

Thank you,, missed this changes

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 59: 3067299-59.patch, failed testing. View results

andypost’s picture

Status: Needs work » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 59: 3067299-59.patch, failed testing. View results

quietone’s picture

Re running tests.

andypost’s picture

Status: Needs work » Reviewed & tested by the community

back to rtbc

quietone’s picture

quietone’s picture

Status: Reviewed & tested by the community » Needs review

Should br NR

Status: Needs review » Needs work

The last submitted patch, 66: 3067299-66.patch, failed testing. View results

quietone’s picture

And update the new migrate tests in aggregator.

andypost’s picture

Thank you

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 69: 3067299-69.patch, failed testing. View results

andregp’s picture

I 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.

andregp’s picture

Status: Needs work » Reviewed & tested by the community

The error on D10 was indeed random, the second test passed so I'm restoring the previous status.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 69: 3067299-69.patch, failed testing. View results

quietone’s picture

Status: Needs work » Reviewed & tested by the community

Failure is Drupal\Tests\quickedit\FunctionalJavascript\QuickEditFileTest::testRemove

Resetting the RTBC

danflanagan8’s picture

Assigned: Unassigned » danflanagan8
Status: Reviewed & tested by the community » Needs review

I'm going to grab this and try the re-roll, per an open request by @benjifisher in Slack.

danflanagan8’s picture

Assigned: danflanagan8 » Unassigned
Status: Needs review » Needs work

nm, @benjifisher took it! Unassigning.

danflanagan8’s picture

I'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

benjifisher’s picture

Status: Needs work » Needs review
FileSize
6.81 KB
9.34 KB

Here 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:

  1. core/modules/aggregator/tests/src/Functional/migrate_drupal_ui/d6/UpgradeTest.php: deleted in 10.0.x, modified here
  2. core/modules/aggregator/tests/src/Functional/migrate_drupal_ui/d7/UpgradeTest.php: deleted in 10.0.x, modified here
  3. core/modules/migrate_drupal/tests/src/Kernel/StateFileExistsTest.php: delete/delete conflict (aggregator and action)

@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.

benjifisher’s picture

I am pretty sure that there are some deprecation tests that have to be removed.

Just one. Patch and interdiff attached.

benjifisher’s picture

I 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.

Status: Needs review » Needs work

The last submitted patch, 80: 3067299-80.patch, failed testing. View results

danflanagan8’s picture

Status: Needs work » Reviewed & tested by the community

That 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.

  • catch committed 9048a7d on 10.0.x
    Issue #3067299 by andypost, iyyappan.govind, quietone, mrinalini9,...

  • catch committed c550b6e on 9.4.x
    Issue #3067299 by andypost, iyyappan.govind, quietone, mrinalini9,...
catch’s picture

Version: 10.0.x-dev » 9.4.x-dev
Status: Reviewed & tested by the community » Fixed

This 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!

andypost’s picture

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.