This extends upon #2454517: Support altering of item CRUD reactions in order to fix an issue with nodes being indexed even with $entity->search_api_skip_tracking = TRUE;.

When saving a node, it follows this path:

  • \Drupal\node\Entity\Node::postSave()
  • \Drupal\node\NodeAccessControlHandlerInterface::acquireGrants()
  • search_api_node_access_records_alter()
  • \Drupal\search_api\Entity\Index::trackItemsUpdated()
  • \Drupal\search_api\Entity\Index::trackItemsInsertedOrUpdated()

And eventually gets indexed in \Drupal\search_api\Entity\Index::indexSpecificItems()

A check should also be added in search_api_node_access_records_alter().

P.S. This can help speed up migrations and reduce the server load during migrations.

CommentFileSizeAuthor
#11 Plugin instances.png24.97 KBsimon georges
#5 2829351-5--node_access_change_skip_tracking.patch718 bytesdrunken monkey
#2 search_api-fix_search_api_skip_tracking_flag-2829351-2.patch544 bytesAnonymous (not verified)

Comments

Anonymous’s picture

Anonymous (not verified) commented

SchnWalter created an issue. See original summary.

Anonymous’s picture

Anonymous (not verified) commented
StatusFileSize
new search_api-fix_search_api_skip_tracking_flag-2829351-2.patch544 bytes
Anonymous’s picture

Anonymous (not verified) commented
Status: Active » Needs review
borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Needs review » Reviewed & tested by the community

Yep, that makes sense.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Priority: Normal » Major
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new 2829351-5--node_access_change_skip_tracking.patch718 bytes

Thanks a lot for reporting this problem! You're right, once again we didn't think of this.
However, your patch would also lead to the node's comments not being updated in indexes, which could be a security problem. It's rather improbable, but in theory there's nothing keeping you from excluding a node from the index, but indexing its comments. And in this case, the comments would still need to know about the node's access records changes.
The attached patch seems better suited to me. Please test/review!

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Needs review » Reviewed & tested by the community

yep

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented

Good we agree.
Committed.
Thanks again, everyone!

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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

simon georges’s picture

simon georges commented
StatusFileSize
new Plugin instances.png24.97 KB

I apologize for coming back to this relatively old issue.

My problem with moving the "skip" in the final patch is that the line !$index->isValidProcessor('content_access') is executed. In our case, it means instantiating a lot of plugins, which account for 10% of the total time of the migration (see attached blackfire screenshot), even when we didn't want to index the content. That don't happen when using the first patch of the thread.

I'm wondering if there is a way to avoid that, or simply if there is a bug somewhere else in my code resulting in instantiating as many plugins.