Saving non-default revisions shouldn't trigger a search reindex

Retitled for the current implementation.

I really need to know if something goes live in hook_node_update(). Otherwise I cannot run code in linkchecker only if the node get's published. I already have troubles with D7 workbench moderation now. Please let us solve this finally for D8.

I think the only bit here that's not addressed yet is search, which is in \Drupal\node\Entity\Node::postSave() now, there are also node_comment hooks that update the search index.

I attached a patch that checks for isDefaultRevision() being set to TRUE before running node_reindex_node_search and same situation for commentInsert or commentUpdate and or commentDelete at /core/modules/node/src/Hook/NodeHooks1.php

I'm starting to do some Core contribution for the first time so bear with me when reviewing.

All contributions must be merge requests.

Created MR to reflect the above comments.

Thanks @berdir , good to know, I'll do that next time.

And @sivaji_ganesh_jojodae thanks for putting my changes in the MR, it looks accurate.

Have not reviewed but issue summary appears incomplete

I improved the issue summary, hope it makes more sense.

Left some comments on the MR.

I documented the $comment parameter, added some more information and created a simple $this->assertTrue() in the testDeterminingChanges() test method (file core/modules/node/tests/src/Functional/NodeSaveTest.php) to test if the node that was updated is the default revision, and prints an informative message.

It seems like a pretty obvious validation but in the real scenario that is what is being checked at ./src/Entity/Node::postSave() before updating the node access table and reindex the search table.

I'm open to any suggestions on this.

Leaving tests tag as the test-only pipeline is passing when I'd expect it to fail.

I refactored the test in a new function,testNodeDefaultRevision(), which checks that previous revisions of the node are not reindexed by looking at the index tables.

https://git.drupalcode.org/project/drupal/-/merge_requests/11353

The test-only pipeline is not passing this time

https://git.drupalcode.org/issue/drupal-1522154/-/jobs/4911642

Rebased and slightly tidied up the test.

Ran testonly feature

1) Drupal\Tests\node\Functional\NodeSaveTest::testNodeReindexDefaultRevision
Failed asserting that actual size 1 matches expected size 0.
/builds/issue/drupal-1522154/core/modules/node/tests/src/Functional/NodeSaveTest.php:245
FAILURES!
Tests: 5, Assertions: 52, Failures: 1.

Shows the coverage

Summary appears to be complete and matches the solution.

Believe all feedback has also been addressed.

Couple of comments on the MR.

Feedback appears to be addressed.

@danrod, thanks for the issue summary update.

Everything appears to be in order here.

Rebased and made some slight tweaks to the test so we don't need to install search module for every other test case in NodeSaveTest

Credits

Couple of questions on the MR, keeping at RTBC for now

One is an obvious typo that I'll self apply

@berdir flagged in #22

there are also node_comment hooks that update the search index

They are as follows:

1. \Drupal\node\Hook\NodeHooks1::commentInsert
2. \Drupal\node\Hook\NodeHooks1::commentUpdate
3. \Drupal\node\Hook\NodeHooks1::commentDelete

But comments only reference a node by entity-id, not entity and revision ID, so in that scenario I think we're always going to have the default revision

Setting back to needs review for someone to confirm my thinking on that

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.

They are as follows:

\Drupal\node\Hook\NodeHooks1::commentInsert
\Drupal\node\Hook\NodeHooks1::commentUpdate
\Drupal\node\Hook\NodeHooks1::commentDelete
But comments only reference a node by entity-id, not entity and revision ID, so in that scenario I think we're always going to have the default revision

I think there are at least theoretical situations where comments are added to forward revisions on sites using content moderation or workspaces. Not sure what the default behaviour is, but a site could certainly be set up to do that - like a special comment field for moderation discussion etc.

Re #56 yeah we reverted the comment changes since you asked back in https://git.drupalcode.org/project/drupal/-/merge_requests/11353#note_51...

But after manually testing, you can indeed comment on a forward revision when CM is enabled. However, as per #54, comments are added by entity id, not revision id, so the comment appears on the published version.

I can't think of a world where this mix of modules and edge cases is actually useful (reindexing a node in core's search when a comment is added to a non-default revision, etc) but I also don't feel like this issue is particularly useful either. I would be happy to save time for everyone and close as a won't fix.

I had absolutely no memory of the MR comment referenced in #57. I think it's technically right to leave the comment behaviour as-is then, even if the use case is weird, it's very unlikely to happen, but also if the comments are added and visible on the default revision, then they might as well update the search index too.

The actual change in the MR seems right so we could just go ahead with that?

I've moved this to "Needs Review" just in case you decide to go ahead with the MR.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. 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.

The solution was updating outdated (and deprecated) code. I've rebased and squashed the MR with the new fix.

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.

Asked a question about the test, if it's complicated we can leave it, but it's worth defaulting to kernel tests instead of slower functional ones I think.

Good call, swapped to a kernel test.

Believe feedback for this one has been addressed and still appears to be working

This needs a rebase for the RunTestsInSeparateProcess attribute

Rebase seems good

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