Problem/Motivation

Follow-up from #2492585-24: Deprecate comment_view() & comment_view_multiple()

Action \Drupal\comment\Plugin\Action\UnpublishByKeywordComment does not implement ContainerFactoryPluginInterface so no way to inject services that used inside.

Proposed resolution

Make the action class implement ContainerFactoryPluginInterface
Add __construct() method with all required services
Use injected services

Remaining tasks

patch, commit

User interface changes

no

API changes

\Drupal\comment\Plugin\Action\UnpublishByKeywordComment implements ContainerFactoryPluginInterface

Data model changes

no

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because clean-up of code introduced in 8.x
Issue priority Normal because reduces fragility
Disruption no
CommentFileSizeAuthor
#6 2526004-6.patch3.71 KBandypost
#6 interdiff.txt3.63 KBandypost
#4 2526004-4.patch2.81 KBandypost
#4 interdiff.txt924 bytesandypost
#1 2526004-1.patch2.62 KBandypost

Comments

andypost’s picture

andypost
he/him
Primary language Russian
commented
Issue summary: View changes
Status: Active » Needs review
StatusFileSize
new 2526004-1.patch2.62 KB

next step to convert \Drupal\comment\Tests\CommentActionsTest::testCommentUnpublishByKeyword to unittest

andypost’s picture

andypost
he/him
Primary language Russian
commented
Related issues: +#874624: Optimization of *_unpublish_by_keyword_action

related needs the same sort of test

Status: Needs review » Needs work

The last submitted patch, 1: 2526004-1.patch, failed testing.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Issue summary: View changes
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt924 bytes
new 2526004-4.patch2.81 KB

Added missed interface, fixed IS

Looks we need to inject string translation and convert t() to $this->t() as well

Probably absence of interface on action could change category to bug.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented

I think it's RTBC just minor points.

  1. +++ b/core/modules/comment/src/Plugin/Action/UnpublishByKeywordComment.php
@@ -21,14 +25,56 @@
+    $this->entityManager = $entity_manager;
...
+    $build = $this->entityManager->getViewBuilder('comment')->view($comment);

    Can we inject CommentViewBuilder instead of entity manager? 

  2. +++ b/core/modules/comment/src/Plugin/Action/UnpublishByKeywordComment.php
@@ -21,14 +25,56 @@
+    return new static($configuration, $plugin_id, $plugin_definition, $container->get('entity.manager'), $container->get('renderer'));

    Please make this multi line.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Assigned: Unassigned » andypost
Related issues: +#2526060: Add tests for action plugins
StatusFileSize
new interdiff.txt3.63 KB
new 2526004-6.patch3.71 KB

Now it's ready for unittesting, working on it

andypost’s picture

andypost
he/him
Primary language Russian
commented
Issue summary: View changes

The test was filed in #2526060-4: Add tests for action plugins let's get agreement on approach, looks we can convert existing tests to kernel test to make it faster 5x.

Also testing of subject does not needed, action uses rendered comment (full) to check for keywords, so if no subject rendered no reason to check keywords in

Updated IS (removed mentions about unit tests)

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
Status: Needs review » Reviewed & tested by the community

Looks good to me.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 6: 2526004-6.patch, failed testing.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented 
Segmentation fault
FATAL Drupal\user\Tests\UserCancelTest: test runner returned a non-zero error code (139).

seems unrelated so re-testing.

Status: Needs work » Needs review

jibran queued 6: 2526004-6.patch for re-testing.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs review » Reviewed & tested by the community

yep, bot flux

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed 268ffb4 and pushed to 8.0.x. Thanks!

I think this is permissible in beta because the disruption is minimal and using the container to inject dependencies is the correct pattern.

  • alexpott committed 268ffb4 on 8.0.x 
    Issue #2526004 by andypost: UnpublishByKeywordComment should inject...
andypost’s picture

andypost
he/him
Primary language Russian
commented
Assigned: andypost » Unassigned

Thanx!

Status: Fixed » Closed (fixed)

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