Move some Action plugins to Action module

I've also moved the UnpublishByKeywordComment. It would be good if you could take a look at it. By the way, the tests aren't running because an old plugin is missing.

Looks like there's something to review here. The test coverage changes will conflict with 📌 Move non-migrations tests to Actions module Needs work .

Think this is postponed until the tests move. Unless we temporarily fix tests here and then fully remove in 📌 Move non-migrations tests to Actions module Needs work but that doesn't sound right.

Since a core committer set this to needs review stating that "there's something here to review". I am setting this back to needs review.

For myself, I see that the moving of the plugins can be removed. And also the remaining tasks identifies more plugins that need to me. The fact that a conflict will happen should not stop this work which we would like to complete in 10.3.

Alright I’ll tag and leave for someone else from the initiative to review. But appears there are test failure.

I don't want to mees up the MR created by @taraskorpach so I started a new one.

Spokje → closed merge request !6240

Spokje → changed the visibility of the branch 3413949-move-some-action-other-approach to hidden.

Sorry for the noise, my approach turned out to be the same approach. Closed and hid my MR.

So the reason the MR fails lies in the migration subsystem, I think NR is a nice status to attract some attention from people with Big Brains who know about that stuff.

📌 Move non-migrations tests to Actions module Needs work has been merged, didn't check to see if this MR is rebased fully but there should be no conflict now but there are currently test failures.

I often forget about the action migrations because they are in the system module. They were failing because the moved plugins were no longer found. To fix this the first step was to prevent the existing action migrations, d6_action and d7_action, from migrating these two plugins (See the use of the static_map plugin in the process pipeline). Then in the action module add a hook_migration_plugins_alter to alter the static map so these plugins will migrate. Tests have been added and changed accordingly for this.

And then all the migration Upgrade tests action entity counts are updated for 2 less Action plugins being migrated.

On reflection, I am not sure why I listed those other plugins to be moved.

On reflection, I am not sure why I listed those other plugins to be moved.

Looking at the IS:

catch pointed out that those two plugins don't make sense without a UI, so we should also consider moving them to the Action UI module. That is what this issue is for.

Where the two mentioned action plugins are node_unpublish_by_keyword_action and comment_unpublish_by_keyword_action.

The other plugins as mentioned by @quietone in #17 📌 Move some Action plugins to Action module Needs review are:

user_add_role_action, user_remove_role_action, node_assign_owner_action which need configuration.

I think the question here is, do these three plugins make sense without a UI?

I think the question here is, do these three plugins make sense without a UI?

I think from the point of view of minimizing disruption, the answer is "yes".

So if it's determined they need a UI what's next steps here?

I did not say that? I said the plugins still make sense even without a UI — because the functionality will keep working even if a UI is only provided after installing a contrib module.

(Similar example for core: https://www.drupal.org/project/restui → )

Just did a review of a MR, had a couple of small nits and some questions around the deprecations

Seems feedback has been addressed in the MR.

For the concern about the namespace it should remain the same once removed to contrib. I checked VBO https://git.drupalcode.org/project/views_bulk_operations/-/blob/4.2.x/sr... who's namespace is Drupal\views_bulk_operations\Plugin\Action;

Is there a follow up for the three other issues mentioned in the IS?

user_add_role_action, user_remove_role_action, node_assign_owner_action

re #25 this actions should stay in core as they are used by user_user_role_insert() and user_user_role_delete

But node_assign_owner_action has tests in action.module so probably should be done here

Deprecated node_assign_owner_action

Moved forgotten config schema to action module
Few tests still fail after deprecation of node_assign_owner_action

Removed strict types declaration as tests started to fail https://git.drupalcode.org/issue/drupal-3413949/-/pipelines/108578

    Drupal\Tests\action\Kernel\UnpublishByKeywordActionTest::testUnpublishByKeywordAction
    TypeError: str_contains(): Argument #1 ($haystack) must be of type string,
    Drupal\Core\Render\Markup given
    
    /builds/issue/drupal-3413949/core/modules/action/src/Plugin/Action/UnpublishByKeywordNode.php:33
    /builds/issue/drupal-3413949/core/lib/Drupal/Core/Action/ActionBase.php:22
    /builds/issue/drupal-3413949/core/modules/system/src/Entity/Action.php:152
    /builds/issue/drupal-3413949/core/modules/action/tests/src/Kernel/UnpublishByKeywordActionTest.php:66
    /builds/issue/drupal-3413949/core/lib/Drupal/Core/Render/Renderer.php:627
    /builds/issue/drupal-3413949/core/modules/action/tests/src/Kernel/UnpublishByKeywordActionTest.php:65
    /builds/issue/drupal-3413949/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

Moved node_assign_owner_action

Checked usage in contrib and the action has only few usages http://codcontrib.hank.vps-private.net/search?text=node_assign_owner_act...

So I updated IS and CR to include node_assign_owner_action

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

rebased

Also fixed both plugins to use new render function 📌 Rename RendererInterface::renderPlain() to ::renderInIsolation() Fixed

It's the last blocker for 📌 Final steps to deprecate Actions module Postponed

Updated the issue summary to show 3 actions are moving and the other 2 are staying in core.

Move of the 3 seems fine

Marking now to get Actions deprecation finalized.

Committed/pushed to 11.x and cherry-picked to 10.3.x, thanks!

I think for user_add_role_action, user_remove_role_action I think it's worth a follow-up to discuss. i.e. for me it's weird that user hooks call out to actions, user can directly use its own APIs in these cases. But that would be at least one issue to refactor user hooks to not depend on actions, then another to move them, so scope here seems fine.

Committed/pushed to 11.x and cherry-picked to 10.3.x, thanks!

Reverted due to:

OK (8 tests, 34 assertions)
Remaining self deprecation notices (6)
  1x: The "Drupal\Tests\UnitTestCase::expectDeprecationMessage()" method is considered internal use expectDeprecation() instead. It may change without further notice. You should not extend it from "Drupal\Tests\comment\Unit\Action\UnpublishByKeywordCommentTest".
    1x in DrupalListener::endTest from Drupal\Tests\Listeners
  1x: The "Drupal\Tests\UnitTestCase::expectDeprecationMessageMatches()" method is considered internal use expectDeprecation() instead. It may change without further notice. You should not extend it from "Drupal\Tests\comment\Unit\Action\UnpublishByKeywordCommentTest".
    1x in DrupalListener::endTest from Drupal\Tests\Listeners
  1x: The "Drupal\Tests\UnitTestCase::expectDeprecationMessage()" method is considered internal use expectDeprecation() instead. It may change without further notice. You should not extend it from "Drupal\Tests\node\Unit\Action\AssignOwnerNodeTest".
    1x in DrupalListener::endTest from Drupal\Tests\Listeners
  1x: The "Drupal\Tests\UnitTestCase::expectDeprecationMessageMatches()" method is considered internal use expectDeprecation() instead. It may change without further notice. You should not extend it from "Drupal\Tests\node\Unit\Action\AssignOwnerNodeTest".
    1x in DrupalListener::endTest from Drupal\Tests\Listeners
  1x: The "Drupal\Tests\UnitTestCase::expectDeprecationMessage()" method is considered internal use expectDeprecation() instead. It may change without further notice. You should not extend it from "Drupal\Tests\node\Unit\Action\UnpublishByKeywordActionTest".
    1x in DrupalListener::endTest from Drupal\Tests\Listeners
  1x: The "Drupal\Tests\UnitTestCase::expectDeprecationMessageMatches()" method is considered internal use expectDeprecation() instead. It may change without further notice. You should not extend it from "Drupal\Tests\node\Unit\Action\UnpublishByKeywordActionTest".
    1x in DrupalListener::endTest from Drupal\Tests\Listeners

https://git.drupalcode.org/project/drupal/-/jobs/1047019

11.x already has it

docker exec \
-u 1000:1000 \
-e SIMPLETEST_BASE_URL=http://172.30.1.2 \
-e BROWSERTEST_OUTPUT_DIRECTORY='db' \
core \
php -dzend.assertions=1 \
vendor/bin/phpunit -c core/phpunit.xml.dist --colors=always --debug \
--group OpenTelemetry
PHPUnit 9.6.15 by Sebastian Bergmann and contributors.

Testing 
Test 'Drupal\Tests\demo_umami\FunctionalJavascript\OpenTelemetryAuthenticatedPerformanceTest::testFrontPageAuthenticatedWarmCache' started
Test 'Drupal\Tests\demo_umami\FunctionalJavascript\OpenTelemetryAuthenticatedPerformanceTest::testFrontPageAuthenticatedWarmCache' ended
Test 'Drupal\Tests\demo_umami\FunctionalJavascript\OpenTelemetryNodePagePerformanceTest::testNodePageColdCache' started
Test 'Drupal\Tests\demo_umami\FunctionalJavascript\OpenTelemetryNodePagePerformanceTest::testNodePageColdCache' ended
Test 'Drupal\Tests\demo_umami\FunctionalJavascript\OpenTelemetryNodePagePerformanceTest::testNodePageHotCache' started
Test 'Drupal\Tests\demo_umami\FunctionalJavascript\OpenTelemetryNodePagePerformanceTest::testNodePageHotCache' ended
Test 'Drupal\Tests\demo_umami\FunctionalJavascript\OpenTelemetryNodePagePerformanceTest::testNodePageCoolCache' started
Test 'Drupal\Tests\demo_umami\FunctionalJavascript\OpenTelemetryNodePagePerformanceTest::testNodePageCoolCache' ended
Test 'Drupal\Tests\demo_umami\FunctionalJavascript\OpenTelemetryNodePagePerformanceTest::testNodePageWarmCache' started
Test 'Drupal\Tests\demo_umami\FunctionalJavascript\OpenTelemetryNodePagePerformanceTest::testNodePageWarmCache' ended
Test 'Drupal\Tests\demo_umami\FunctionalJavascript\OpenTelemetryFrontPagePerformanceTest::testFrontPageColdCache' started
Test 'Drupal\Tests\demo_umami\FunctionalJavascript\OpenTelemetryFrontPagePerformanceTest::testFrontPageColdCache' ended
Test 'Drupal\Tests\demo_umami\FunctionalJavascript\OpenTelemetryFrontPagePerformanceTest::testFrontPageHotCache' started
Test 'Drupal\Tests\demo_umami\FunctionalJavascript\OpenTelemetryFrontPagePerformanceTest::testFrontPageHotCache' ended
Test 'Drupal\Tests\demo_umami\FunctionalJavascript\OpenTelemetryFrontPagePerformanceTest::testFrontPageCoolCache' started
Test 'Drupal\Tests\demo_umami\FunctionalJavascript\OpenTelemetryFrontPagePerformanceTest::testFrontPageCoolCache' ended


Time: 00:08.185, Memory: 144.00 MB

OK, but incomplete, skipped, or risky tests!
Tests: 8, Assertions: 0, Skipped: 8.

Remaining self deprecation notices (1)

  1x: The Drupal\Tests\field\Traits\EntityReferenceTestTrait is deprecated in drupal:10.2.0 and is removed from drupal:11.0.0. Instead, use \Drupal\Tests\field\Traits\EntityReferenceFieldCreationTrait. See https://www.drupal.org/node/3401941
    1x in FunctionalTestSuite::suite from Drupal\Tests\TestSuites

It was old ctools in modules

Follow-up filed 📌 Stop using action in user module in role entity hooks Active (also found few related issues)

Thanks @andypost for the removal of just the ExpectDeprecationTrait.

Also opened 📌 [PP-1] Move configurable user actions to the actions module Active as a follow-up to the follow-up, we can always decide not to do that, but I think it's worth discussing in its own issue (and also not trying to do here because it's not as simple).

Committed/pushed to 11.x and cherry-picked to 10.3.x, thanks!

Thanks, looks everything fine now and CR can be published

Please close the MR and the remaining issue is 📌 Final steps to deprecate Actions module Postponed

catch → closed merge request !6120

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