Search links cause many nodes to be indexed twice

+++ modules/search/search.module	20 Sep 2009 15:42:44 -0000
@@ -678,7 +678,7 @@
+    else if ($sid != $nid || $type != 'node') {

The style guide favors elseif over else if here.

I'm on crack. Are you, too?

Rerolled with the elseif fixed and also added the tests from #581594: minor db_query() fix to search for DB TNG.

Looks great.

Yay! Critical bug fixes with tests. Yum, my favourite! :)

Could we get a one or two line comment along with the fix though? There's a lot of good info in this issue about the bug which could stand to be captured inline so someone doesn't try and 'optimize' this later.

Anyone? webchick is only asking to clarify docs. 2 minute job! ;)

It's not a 2-minute job any more, because the patch above doesn't apply...

#7: 569536_581594.patch queued for re-testing.

Here's a re-roll of the patch in #7, with an expanded comment. Hopefully I've captured what webchick was hoping to see in that comment...

Hmmm, newline problem, try this one.

I couldn't reproduce this bug, so I'm not sure what it's supposed to fix. Can anyone provide steps to reproduce? Also the attached test passes without the fix to search.module, so the test doesn't seem to relate to the bug.

I tried adding a watchdog statement that would log index operations that were skipped by this extra conditional, and it was never triggered. I also checked the contents of the search tables both with and without this patch and they looked the same to me.

The tests came from a different issue, I gather -- see #7. They should really be in a separate patch, but here they are...

We should probalby add a test to verify that this patch actually fixes the issue...
The problem, as I understand it:

a) Make a node that links to itself.
b) Index that node.
c) During indexing, links in the text of the node being indexed, linking to other nodes, cause the linked-to node to be added to the search_node_links table and to be marked for reindexing.
d) This shouldn't happen if the link is to the node being indexed, but under the old code, it was.

jhodgdon, thanks for the clarification! Here's an updated patch that tests for the bug itself. This test will now fail without the patch to search.module.

Uploading just the test from #18 so testbot can fail it.

Patch looks good so #18 is RTBC.

1 week, 2 days, patch might be out of date, retesting.

#18: 569536_search_twice.patch queued for re-testing.

Just to clarify, #19 is catch just uploading the test so we could verify that the test fails without the patch (which it did). #18 is the patch that should be committed (which passes tests).

Looks good, and glad to see we added tests. Committed to CVS HEAD.

We should consider this for D6 back-port as well, I think.

Is this issue still needed?

The fix has not been ported to Drupal 6, so yes, the issue is still needed until a decision is made.

D6 backport.

Please read the help text under attachments. If you name patches like that, the test bot will ignore them. :)

Anyway. I'm not doing search.module maintaining any more, so I'm unfollowing myself from this issue... I don't think anyone is actually maintaining search.module actually. So I apologize if the probable lack of reviews on this issue makes you think no one appreciates your efforts... :(

@jhodgdon my feelings aren't hurt at all. Just trying to whittle down the number of open tickets!

I named the file like that because it's for D6. I thought that was the standard practice since there aren't tests for D6?

Renaming so testbot will be happy.

Wow, looking through old issues...

This fix was carefully tested in 7.x and this is a straight backport of it to 6.x. I think we should consider adding this to 6.x.

Talked with Gabor (the Drupal 6 branch maintainer) and D6 issues are really not being committed unless they're really essential -- we really don't have a test system for Drupal 6 and it's too dangerous. So... putting this back to D7 / fixed.