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 #3369914: Move non-migrations tests to Actions module.

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

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.

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.

#3369914: Move non-migrations tests to Actions module 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.

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 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 mean that — sorry for not being more clear.

I meant 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 #2511308: Rename RendererInterface::renderPlain() to ::renderInIsolation()

It's the last blocker for #3369912: Final steps to deprecate Actions UI (action) module

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 #3427413: Stop using action in user module in role entity hooks (also found few related issues)

Thanks @andypost for the removal of just the ExpectDeprecationTrait.

Also opened #3427549: [PP-1] Move configurable user actions to the actions module 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 #3369912: Final steps to deprecate Actions UI (action) module