In node module, we have this comment:

// Remove this node from the search index if needed.
// This code is implemented in node module rather than in search module,
// because node module is implementing search module's API, not the other
// way around.

Which precedes a search_wipe() wrapped in if (module_exists('search')) {

I think that it would be cleaner for search to implement hook_nodeapi_delete() and do the search wipe for the node there, rather than having module_exists().

Patch is attached ...

CommentFileSizeAuthor
search-wip.patch1.63 KBkbahey

Comments

david strauss’s picture

david strauss
he/him
commented

I'm not sure I agree with just moving the functionality to the search module, but I do agree we need looser coupling with the search module.

kbahey’s picture

kbahey commented

This is not about "just moving".

Rather, hookifying things with nodeapi when a node is deleted, where they really belong.

This way things are cleaner, and done the Drupal way.

david strauss’s picture

david strauss
he/him
commented

Is search intended to only index users and nodes, or is it supposed to be more flexible? If it's supposed to be more flexible, then the proposed change doesn't fix the underlying architectural problem of search needing special code to respond to events for each type of item it indexes. If search exclusively indexes nodes and users, this solution is probably acceptable.

kbahey’s picture

kbahey commented

I don't intend to rearchitect the search module. This patch puts the code where it really belongs, instead of having conditionals in other modules.

By putting this patch in search we are not precluding any future improvements either.

Like you said, decoupling is a good thing.

david strauss’s picture

david strauss
he/him
commented

@kbahey Agreed.

Status: Needs review » Needs work

The last submitted patch failed testing.

Jooblay.net’s picture

Jooblay.net commented
Issue summary: View changes

What is the status of this ticket:) Can we close this...

Status: Needs work » Closed (outdated)

Automatically closed because Drupal 7 security and bugfix support has ended as of 5 January 2025. If the issue verifiably applies to later versions, please reopen with details and update the version.