Optimization of *_unpublish_by_keyword_action

Patch looks good but are these actions tested at all? If they are then great, but going by #244093: Node and comment actions are (still) completely broken and have broken tests too a lot of actions aren't.

#764558: Remove Trigger module from core has been committed, so moving back to D7. Tagging for test coverage based on #6. Thanks everyone!

Uploading a test and a patch against d8 to cover off the comment part of this issue. I had to change the original patch slightly since it did not work properly.

There are currently no tests for node actions, so looks like a bit more work needs to be done there. If I can, I will try uploading a separate patch for the node issue along with node actions tests.

woops, didnt notice the d8 comment about actions disappearing (re-rolling against 7)

And here's the d7 patches for comment.

+++ b/modules/comment/comment.moduleundefined
@@ -2592,9 +2592,12 @@ function comment_unpublish_action($comment, $context = array()) {
+   ¶

code style problem.

#11: 874624-d7-comment.patch queued for re-testing.

adding novice tag for redoing patches for taking out the extra white space... and

+++ b/modules/comment/comment.testundefined
@@ -1969,6 +1970,13 @@ class CommentActionsTestCase extends CommentHelperCase {
     $this->assertEqual(comment_load($comment->cid)->status, COMMENT_PUBLISHED, t('Comment was published'));
     $this->assertWatchdogMessage('Published comment %subject.', array('%subject' => $subject), t('Found watchdog message'));
...
+    $this->assertWatchdogMessage('Unpublished comment %subject.', array('%subject' => $subject), t('Found watchdog message'));

t() in the assert might not be needed.

http://drupal.org/node/500866

Patch fo d7 corrected.

@babruix Interdiffs are awesome. For next time. :)
For instructions on creating an interdiff, see http://drupal.org/node/1488712 Or, see http://xjm.drupalgardens.com/blog/interdiffs-how-make-them-and-why-they-...

I think this has tests... so removing the needs tests tag.

This is a patch for the Comment module, not the Trigger module. So it still needs to go into Drupal 8.

Looking at the code, it's possible this is partially fixed in Drupal 8 already, but I'm not sure if it's completely fixed... Also the test changes definitely don't look like they're in Drupal 8 yet.

Hm, also, there were changes to the node module too in earlier patches... what happened to those?

Starting with this as part of the Prague mentoring sprint

There are two things to deal with here.

1. Whether the fix in comment_unpublish_by_keyword_action in the D7 patch in #20 has come into D8
2. What happened to the node_unpublish_by_keyword_action code in the original patch in #1

1. Whether the fix to the code in comment_unpublish_by_keyword_action in the D7 patch in #20 has come into D8

Appropriate code in D8 is in UnpublishByKeywordComment.php:

  public function execute($comment = NULL) {
    $build = comment_view($comment);
    $text = drupal_render($build);
    foreach ($this->configuration['keywords'] as $keyword) {
      if (strpos($text, $keyword) !== FALSE) {
        $comment->status->value = COMMENT_NOT_PUBLISHED;
        $comment->save();
        break;
      }
    }
  }

As you can see the key line "$text = drupal_render($build);" has indeed been moved outside of the foreach. Also the action is tested in CommentActionsTest:testCommentUnpublishByKeyword. Also, the D7 patch passes all tests and has been reviewed so I think we can conclude that the D7 fix is already in D8 and no action is needed on the comment_unpublish_by_keyword_action function.

2. What happened to the node_unpublish_by_keyword_action code in the original patch in #1

The code for node_unpublish_by_keyword_action has indeed disappeared from the original D7 patch in #1 and the improvement has not been made in D8. In D8 the node is still being rendered inside the foreach loop

I have attached a patch with the following:

1. Changed node_unpublish_by_keyword_action to use the same higher performing approach in comment_unpublish_by_keyword_action
2. Added a new test case in a new file in modules/node/Tests/NodeActionsTest.php to match the location of modules/comment/Tests/CommentActionsTest.php. I searched for an existing Node actions test and could not find it.

Minor styling improvements

Styling changes

I noticed there was no test coverage over Node actions. See #1412964: Add additional test coverage for actions

Re-rolling patch against latest HEAD

novice tag was added a while back for #16 and that was done. so removing novice tag. https://www.drupal.org/core-mentoring/novice-tasks

doing https://www.drupal.org/contributor-tasks/update-allowed-beta
evaluating for https://www.drupal.org/contribute/core/beta-changes

taking a look at this now...

I have made a mistake in the new test. so this is not passing at the moment. The problem is to do with which user to use while filtering!

I am posting ... before going to bed ... maybe someone could nudge me in the right direction ... otherwise I will look again with fresh eyes in the morning

#38.1 use preferred dependency injection ... done.

#38.2 refactor for EntityUnitTestBase...

some notes

1) @group in needed now...
2) entity_create is deprecated.. I have replaced with Action::create() and Node:create()
3) drupalCreateUser became createUser in the new test environment.
4) Yep somehow I have muddled the simulation of logging in in the $admin_user.

Ok next iteration,

1) I have removed the format definition it was not needed... (this was filtering issue from #40)
2) Creating a node without 'type' or 'title' triggers integrity violations. so I have inserted dummy values.
3) We can remove all the login fluff ... it was not needed.

Now for the question

when the action operates on the node in the test

  $action->execute(array($node));

some rendering is required ...

Is there a standard pattern for faking this in a low level test?

otherwise we are going to have to rethink the plan to use EntityUnitTestBase.

can we fake a render context?

currently the error is

LogicException: Render context is empty, because render() was called outside of a renderRoot() or renderPlain() call. Use renderPlain()/renderRoot() or #lazy_builder/#pre_render instead. in Drupal\Core\Render\Renderer->doRender() (line 246 of /Users/martin/sites/drupal/core/lib/Drupal/Core/Render/Renderer.php).

#44.1 Converted node_view to $this->entityTypeManager->getViewBuilder('node')->view($node);

#44.2 Converted to renderPlain() thanks that seems to be the correct thing to do. Not sure about the new error message yet .. just posting early.
Its seems something in the node upsets the rendering.

triggering testbot

Thank you for creating this issue to improve Drupal.

We are working to decide if this task is still relevant to a currently supported version of Drupal. There hasn't been any discussion here for over 8 years which suggests that this has either been implemented or is no longer relevant. Your thoughts on this will allow a decision to be made.

Since we need more information to move forward with this issue, the status is now Postponed (maintainer needs more info). If we don't receive additional information to help with the issue, it may be closed after three months.

Thanks!

wanted to bump this 1 more time.