Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Hi,
I have been playing with the Solr search module and hit a problem I have traced back to the core search module.
The problem: On some search results the description ends with '()'
The cause: search_nodeapi 'update index' hook is returning a imploded array wrapped in a anchor tag without checking the array size is > 0.
Problem code line 631 search.module
/**
* Implementation of hook_nodeapi().
*/
function search_nodeapi(&$node, $op, $teaser = NULL, $page = NULL) {
switch ($op) {
// Transplant links to a node into the target node.
case 'update index':
$result = db_query("SELECT caption FROM {search_node_links} WHERE nid = %d", $node->nid);
$output = array();
while ($link = db_fetch_object($result)) {
$output[] = $link->caption;
}
return '<a>('. implode(', ', $output) .')</a>';
// Reindex the node when it is updated. The node is automatically indexed
// when it is added, simply by being added to the node table.
case 'update':
search_touch_node($node->nid);
break;
}
}
Chainging it to the below fixes my error of '()' appearing in search results:
/**
* Implementation of hook_nodeapi().
*/
function search_nodeapi(&$node, $op, $teaser = NULL, $page = NULL) {
switch ($op) {
// Transplant links to a node into the target node.
case 'update index':
$result = db_query("SELECT caption FROM {search_node_links} WHERE nid = %d", $node->nid);
$output = array();
while ($link = db_fetch_object($result)) {
$output[] = $link->caption;
}
if(count($output >=1) {
return '<a>('. implode(', ', $output) .')</a>';
}
// Reindex the node when it is updated. The node is automatically indexed
// when it is added, simply by being added to the node table.
case 'update':
search_touch_node($node->nid);
break;
}
}
Comment | File | Size | Author |
---|---|---|---|
#8 | add-break-376408-8-D6.patch | 697 bytes | pwolanin |
#1 | empty-parens-376408-1.patch | 642 bytes | pwolanin |
#1 | empty-parens-376408-1-D6.patch | 835 bytes | pwolanin |
Comments
Comment #1
pwolanin CreditAttribution: pwolanin commentedThanks for tracking this down!
Comment #2
ajevans85 CreditAttribution: ajevans85 commentedSomething was bugging me about this so I have just had another look.
The patch does fix the problem of the '()' appearing at the end of searches.
However....
if the below statement returns true a non schematical html string is returned....
The above isn't a valid anchor tag, it's not linking to anything!
As the returned value is inserted within a paragraph this may be a better option
Comment #3
ajevans85 CreditAttribution: ajevans85 commentedSorry, I get a error about selecting a valid project when trying to edit my last comment.
My proposed fix should read
Comment #4
pwolanin CreditAttribution: pwolanin commentedI'm guessing an <a> tag is used is because of the way search module biases results. " Transplant links to a node into the target node."
Let's not change that for now - this content is only used for the search index.
Comment #5
Gábor HojtsyTrivial fix. Committed to Drupal 6, and should go to Drupal 7 as well.
Comment #6
BoobaaBefore the patch the 'update index' case was returning unconditionally. Now it returns a string only there is something to return; if there is not, it runs into the next ('update') case, as there is no break statement (at least in the D6 version).
Is it intentional and right, and I'm just a bit over-zealous, or the patch is not as good as it meant to be?
Comment #7
Gábor HojtsyWell, search_nodeapi() $op 'update index' is called via the node_invoke_nodeapi() in http://api.drupal.org/api/function/_node_index_node/6 After getting data from these $op-s, it finally calls http://api.drupal.org/api/function/search_index/6 which turns the reindexing bit of the node back to 0 if all data was cool (or in itself, it might touch the node again for reindexing), just like the search_nodeapi() $op "update" does. So while it is definitely not nice to touch the node again at this point, it does not seem like to make any immediate problems by the looks of the code.
I totally agree however that it should be fixed first, and then ported to D7, since we have this committed to D6 unfortunately.
Comment #8
pwolanin CreditAttribution: pwolanin commentedAdditional fix for D6.
Comment #9
pwolanin CreditAttribution: pwolanin commentedD7 patch in #1 is ok, due to ops being split to different functions
Comment #10
Gábor HojtsyCommitted to D6, so moved to D7 for commit of #1.
Comment #11
sun.
Comment #12
webchickCommitted to HEAD. Thanks!