Implementations of hook_node_update should check the "isDefaultRevision" property

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 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.

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 check 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.