After deleting a translated article, search wants to reindex it

I tested on Drupal 11.x and, using the steps in the Issue Summary, I can confirm the problem still exists.

zniki.ru → made their first commit to this issue’s fork.

Provided tests to display described issue.
I created draft MR.

I'm not seeing an option to remove the draft status from MR.

sukr_s → changed the visibility of the branch 3208247-after-deleting-a to hidden.

unable to change the draft status in MR so reverted the changes and created a new mr.

Not sure a hook is the best place for this. There's NodeSearch.php shouldn't that leveraged to handle a deletion. Or does content_translation need a search plugin as well?

When a node is deleted, it's removed from the search index in the node::preDelete event. It's not handled by NodeSearch or content_moderation. On the same lines when a translation is deleted, it must be removed from the search index using the translation_delete hook.

Still think there could be a better way to go about it. But at a minimum believe the change and test should belong in the search module. Not all sites use the search module and even though this hook wouldn’t do anything it will still get called every translation deletion.

w.r.t to alternate approach to solve this, it would be far more expensive to find what was deleted in a cron run e.g. run though list of languages configured for the site, then the list of languages currently available to for the node and then to clear the index for missing languages. So I believe it's best done at the time of deletion.

I also think that this should be in search module. However this would result in hard coding the type node_search (used in clear method) in search module which would not be right since search module does not own the 'node_search' plugin. I could not find a method to dynamically determine the search plugin type based on entity type. Therefore chose to place it in the node module.

is it possible to find search plugin type based on entity type or is it ok to hard code the type in search module?

Will see if the sub maintainer takes a look in a few weeks.

Node module is also responding to various changes in hooks (e.g. comment update) and calling node_reindex_node_search()

This feels like a similar case, so I think the approach in the MR is reasonable given the limitations of the system.

Thanks @pwolanin for taking a look!

https://git.drupalcode.org/issue/drupal-3208247/-/jobs/1453892 shows the test coverage

Applied 1 more nitpicky change but this got sign off from the sub-maintainer so seems fine solution that addresses the problem.

Seems tests are failing.

smustgrave → changed the visibility of the branch 11.x to hidden.

I have 0 idea how the suggestion of :void broke so many tests. But started a new MR as the branch name 11.x wasn't good for local (since that's the main branch). Applied the suggestion by @alexpott and tests are green again.

Still very weird.

Sorry @sukr_s @alexpott for all the noise

I think we should open a follow-up to add a post update to clean up the incorrect data. This issue stops the rot but the bogus data hangs about.

Opened 📌 Post update for clearing search index of deleted translations Active

One question on the MR.

Actually two questions.

Nodes are marked for reindex in Node::postSave()

   // Reindex the node when it is updated. The node is automatically indexed
    // when it is added, simply by being added to the node table.
    if ($update) {
      node_reindex_node_search($this->id());
    }

If we could detect that a translation is being deleted there (can we?) then we would not need to mark the node for indexing, and then delete it again in the hook implementation added here.

Or... is there somewhere else that has this information instead?

changed to postsave and invoking node_reindex_node_search($this->id()); only if the translation was not deleted.

Left a comment on the MR. I think we need to always mark for reindex. The reindexing is not language specific but doing this is not going to recreate any row.

Committed/pushed to 11.x and cherry-picked back through to 10.3.x, thanks!

catch → closed merge request !7966

I missed a test failure in the last pipeline here, which then broke HEAD, reverted from all four branches.

"commit db83ea46 - Better names and no need to pass translations around" broke the fix, which has now been resolved by reverting the relevant changes.

Edit.

Tests appear green again.

I read the issue summary, comments and the MR. The found two things that need attention

2) This needs a follow up for https://git.drupalcode.org/project/drupal/-/merge_requests/7966#note_310136

@quietone the approach changed after that comment and is no longer relevant.

I've moved the private function to be inline - keeping old translation objects around after \Drupal\Core\Entity\ContentEntityBase::postSave feels wrong.

Left a comment on the MR - is there really no way to do this from search module or similar, putting it in a specific entity class feels wrong - what about other translatable nodes?

@larowlan all the search integration for nodes is in node module, like the NodeSearch plugin. I agree it would be much nicer if search module had entity support which was then enabled for nodes by default, but for now this should stay with the rest of the integration.

I think catch's answer in #57 menas this can go back to rtbc.

I tested this on 11.x today and found that this does remove the deleted item from the search index, but only after a cron run. It still adds it to the search index, so that the search page shows 98% of the site has been indexed. There is 1 item left to index. after the deletion. That is incorrect. And not what I expected from reading the issue title and summary. I expected search to not want to re-index it.

There are the steps I used with the diff applied.

1. admin/config/search/pages - 100% of the site has been indexed. There are 0 items left to index.
2. Create article
3. admin/config/search/pages - 98% of the site has been indexed. There is 1 item left to index.
4. Run cron
5. admin/config/search/pages - 100% of the site has been indexed. There are 0 items left to index.
6. Add translation
7. admin/config/search/pages - 98% of the site has been indexed. There is 1 item left to index.
8. Run cron
9. admin/config/search/pages - 100% of the site has been indexed. There are 0 items left to index.
10. Delete translation
11. admin/config/search/pages - 98% of the site has been indexed. There is 1 item left to index.
12. Run cron
13. admin/config/search/pages - 100% of the site has been indexed. There are 0 items left to index.

If my testing is confirmed correct then it looks like a follow up is needed for correcting the information on /admin/config/search/pages. Or have I misunderstood something again on this issue?

When any translation is removed, the original content is also added for index. This behaviour has not changed. Pervious to this fix, after the cron run the original article would get reindexed but the removed translation would be left hanging and still gets reported as "1 item requires indexing".
After this fix, the translation is removed from the index therefore before the cron run it shows 1 item which is the original article and after the cron run, it's zero items.

#60 makes sense to me, the entity will be re-indexed but the translation won't.

Since this caused a regression before, committed to 11.1.x and cherry-picked to 10.4.x, thanks!

quietone → closed merge request !8211

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