I have set up a view with a context search filter (search terms). So putting just 1 keyword as argument, the view gives me results. But if there are more words, there are always no results. I am 100% sure that this view must return results. When I use same search terms in "search/node/key1 key2..." the search gives me the results. There are no filters or other methods which exclude my wanted result items.

Here is the SQL statement with 1 keyword:

SELECT node.nid AS nid, node.title AS node_title, node.created AS node_created, SUM(search_index.score * search_total.count) AS score, 'node' AS field_data_field_largeimages_node_entity_type
FROM 
{node} node
LEFT JOIN {search_index} search_index ON node.nid = search_index.sid
LEFT JOIN {search_total} search_total ON search_index.word = search_total.word
WHERE ((( (search_index.type = 'node') AND( (search_index.word = 'example') )))AND(( (node.status = '1') AND (node.type IN  ('article')) AND (node.created >= 1338764465-3456000) )))
GROUP BY search_index.sid, score, nid, field_data_field_largeimages_node_entity_type, node_title, node_created
HAVING (( (COUNT(*) >= '1') ))
ORDER BY node_created DESC

which returns results.

This is a statement with 2 keywords, returning nothing but should:

SELECT node.nid AS nid, node.title AS node_title, node.created AS node_created, SUM(search_index.score * search_total.count) AS score, 'node' AS field_data_field_largeimages_node_entity_type
FROM 
{node} node
LEFT JOIN {search_index} search_index ON node.nid = search_index.sid
LEFT JOIN {search_total} search_total ON search_index.word = search_total.word
WHERE ((( (search_index.type = 'node') AND( (search_index.word = 'example1') OR (search_index.word = 'example2') )))AND(( (node.status = '1') AND (node.type IN  ('article')) AND (node.created >= 1338764592-3456000) )))
GROUP BY search_index.sid, score, nid, field_data_field_largeimages_node_entity_type, node_title, node_created
HAVING (( (COUNT(*) >= '2') ))
ORDER BY node_created DESC

Attached a view export.

Thanks for your help.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mxh’s picture

I just tested with stable 7.x-3.3 and there it works fine. So I just leave this issue as active that this problem won't be ignored in the dev branch. I tested this on another D7.14 installation and the problem also occurs there with the dev version from June 1st.

I think the problem is somewhere in the source code, not in the SQL statement.

Unfortunately I cannot work with 7.x-3.3 because this version breaks a lot of my views, which are correctly working (except this problem of course) in the dev version. So I'd be very thankful if a maintainer could look into this.

Thank you for your help.

Update: There could be some results when using more than 1 word, but specific node types are just not being recognised. Example: Some nodes of type A are returned in result set, but there are no results of type B - but there should, and this works in 7.x-3.3.

mxh’s picture

I'd be very thankful if someone cares about it and would give some feedback. The problem is still there in the current dev version. I hope it won't be present in the upcoming stable releases.

mxh’s picture

Version: 7.x-3.x-dev » 7.x-3.5

So the problem is now in the current stable release.

Short summary:
When using more than 1 word for argument, I get no results in the view, but on previous version (7.x-3.3) it works just fine.
The SQL statement is the same in 7.x-3.5 and 7.x-3.3, so there must be something wrong in the source code of the Views module.

I have noticed that the SQL statement created from the module is slightly different from 7.x-3.5 to 7.x-3.3:

In 7.x-3.3, where it worked fine, the "GROUP BY score" is missing. In 7.x-3.5 this new GROUP statement appears. I don't know what this is for.

mxh’s picture

Title: Search terms: no results when using 2+ words » Search terms: nodes dont appear when using multiple arguments
Priority: Normal » Major

I really need help on this. I can't figure out where the origin of this problem is in the source code.

jtwalters’s picture

I have this same issue with views 7.x-3.5

Further testing shows a regression was introduced in 7.x-3.4. Multiple search terms works as intended with views 7.x-3.3

mxh’s picture

On a completely fresh Drupal installation, just installing Views 7.x-3.5 and Ctools, and creating some nodes, there it works fine. Only on my updated sites (from 7.x-3.3 to latest dev and stable) the problem occurs. I hope the bug is reproducable for the maintainers.
I searched for days the source code and have given up so far, no success :/. I need a tip where the trigger of this problem could be.

Please help.

jefferius’s picture

I had the same problem with views 7.x-3.5, my exposed view search for my blog would work if one word is used, but if two words are used the views search does not work - no results.

I have gone back to 7.x-3.3 and it works fine again.

awolfey’s picture

funkytraffic’s picture

+1 Same error here
1 word all results expected (11)
2 words 9 results of 11
4 words 0 results of 11

Anonymous’s picture

Noticing the same issue.

Tracked it down to views_plugin_query_default.inc and the following code

if ($this->has_aggregate && (!empty($this->groupby) || !empty($non_aggregates))) {
	$groupby = array_unique(array_merge($this->groupby, $non_aggregates));
	foreach ($groupby as $field) {
		$query->groupBy($field);
	}
	if (!empty($this->having) && $condition = $this->build_condition('having')) {
		$query->havingCondition($condition);
	}
}

For whatever reason, this code is adding an extra GroupBy for every field in the query that doesn't use an aggregate function. By doing this, the HAVING (COUNT(*) ...) added by views_handler_filter_search.inc is nullified because the results are no longer grouped solely by search_index.sid which is needed to determine how many word matches there are.

With the following change, to not add any extra fields to GroupBy if we have one already, I have it working for me(sorry, not good with patches).

if ($this->has_aggregate && (!empty($this->groupby) || !empty($non_aggregates))) {
  // don't mess with the original groupby if it was set
  if (!empty($this->groupby)) {
    $groupby = $this->groupby;
  }
  // use the non aggregates to create our groupby
  else {
    $groupby = $non_aggregates;
  }
	
  foreach ($groupby as $field) {
    $query->groupBy($field);
  }
  if ($condition = $this->build_condition('having')) {
    $query->havingCondition($condition);
  }
}

I didn't want to drop the $non_aggregates altogether, but I can't see why they'd be used anyway. No matter what sort of query you are running, adding extra fields to groupby will always add the chance the results will not be grouped properly.

salbertz’s picture

Confirmed.

Ran into the same problem and also tracked it down to the - apparently superfluous - GROUP BY columns added by the query plugin itself (and therefore unresponsive to any hook-hacking), before I found this thread.

These additionally merged in columns:
$groupby = array_unique(array_merge($this->groupby, $non_aggregates));
seem to nullify the HAVING-clause and thus cause valid search results to disappear. And I cannot imagine what it should be good for... Getting rid of it in the above manner seems to fix the problem immediately.

jmking’s picture

Confirmed here as well, but different symptoms. The fix in #10 solved my issue.

I was having the problem where duplicate results were showing up for each matched result. I tracked it down to the new grouping by "score" added in 3.4 or 3.5.

score is expressed as SUM(search_index.score * search_total.count) in the query - this causes there to be a unique score field for each hit in the search index causing several duplicates to show up depending on the number of hits.

I also can't figure out why this was added, or what it tries to solve. Has anyone found the issue where this was introduced? I've been searching around, and can't find it.

awolfey’s picture

Version: 7.x-3.5 » 7.x-3.x-dev
Status: Active » Needs review
FileSize
1.03 KB

This works for me. Here's a patch for the changes in #10.

mxh’s picture

The problem is why the code exists? There must be a reason that someone added an extra GroupBy.
I'm worried about if patch from #13 will be commited, that this will cause another problem somewhere, because some little feature needs this extra GroupBy..

salbertz’s picture

Hmm, let's see... The code in the current version (which is cleary wrong - and I would consider the missing search results for multiple terms a major bug, which should IMHO take priority over some hypothetical exotic minor feature "somewhere out there"...) groups by $this->groupby and $non_aggregates, while the patch groups either by $this->groupby or $non_aggregates. So the change will only matter in cases when $this->groupby as well as $non_aggregates are non-empty (the original condition).

A $this->groupby seems to be used mainly by the views/modules/search stuff (e.g. the said grouping by search_index.sid in views/modules/search/views_handler_argument_search.inc l. 94). The other cases I encountered (grepping for 'add_groupby()') were the $this->distinct feature (in views_plugin_query_default::query()) and the summary function (views/handlers/views_handler_argument.inc l. 886).

The former (in my experience) does not work in every case anyway (and if it does, it should take priority over everything else, if I understand that feature right...). And the latter does not seem to use the code in question...

As on the other hand $non_aggregates only become relevant, if "use_aggregation" is selected, the sole case in which problems could arise (exotic modules' features again not taken into account), would be when using a search_index based search with aggregation.

Since I never did that (and would consider it a rather untypical use of the search functionality), I tried to construct a view that uses the search index and aggregation, and noticed (for the first time!) that, as soon as you select "use aggregation", the field "search score" is no longer available (under "Fields -> add field") in the views UI. So the designers clearly seem to have tried to prevent that "pathological" case.

(It is nevertheless possible to construct the view by first adding the "search.score" field and the "search terms" exposed filters (and switch on aggregation afterwards) - but then the aggregation is spoiled anyway (since "GROUP BY search_index.sid" is added in any case), and again the patched version seems to be the only one that yields correct results for multiple search terms.)

For me the patch seems to work perfectly.

mxh’s picture

Status: Needs review » Needs work

Patch from #13 needs to be cleaned up due to Drupal's coding standards.

Is there a Views maintainer who can give further feedback on this? Thanks in advance.

salbertz, thank you very much for your opinion, but I'd like to have a Views maintainer taking a look over this, furthermore we need a maintainer who will commit the proper patch.

salbertz’s picture

Sure. Would it be useful to change the issue status to catch a maintainer's attention - to "needs review" or "patch" or something like that? (I guess you know the conventions better than me to judge what would be the most appropriate.)

The issue does not seem to be too popular (maybe not too many using complex search condition in views exposed filters), though I'd consider it a rather heavy bug and would like to see it fixed in the nearer future...

awolfey’s picture

Status: Needs work » Needs review

Hauptm, can you please identify the standards errors you found? It looks fine to to the coder module. You could make an argument that the comments should be proper sentence case, but I'm not seeing anything else. In any case, whoever commits can adjust that before committing.

I've been using this in production on an enterprise-level site since it was posted. Can anyone else confirm that this is RTBC? That will get the maintainers' attention.

Thanks.

mxh’s picture

You could make an argument that the comments should be proper sentence case, but I'm not seeing anything else.

Yes, this should be everything which has to be cleanded up on this.

I also confirm that patch from #13 solves my posted issue.

though I'd consider it a rather heavy bug and would like to see it fixed in the nearer future...

Agreed! Another reason could be that this bug is not immediately apparent but a critical one for production sites :-/

Many thanks to r_smylski, salbertz and awolfey for helping me out on this one, nice work!

steven.wichers’s picture

The reason for the GROUP BY is most likely due to differences in MySQL and PostgreSQL as well as the different SQL standards. If you remove the functionality then Views will likely break when used on a PostgreSQL based site.

salbertz’s picture

The functionality (of grouping by $non_aggregates) is not removed at all in the general case (as I tried to illustrate in #15) - only if "search terms" (or something else) provides a $this->group_by of its own, this takes precedence (in the patched version) over the grouping by $non-aggregates, instead of being combined with it (which still simply does not seem to make sense to me).

But I don't have any experience with PostgreSQL - if you (or anyone else) have: is it really imaginable, that the query produced by the old version ("GROUP BY search_index.sid, col1, col2, col3....." - including virtually any column used by the view, since one wouldn't typically use any aggregate function in a search) can be correct - or even more - necessary for PostgrSQL?

Anonymous’s picture

The issue is that mysql is spoiling us. Like mentioned in #20, true SQL requires that every column mentioned in the SELECT expression, that isn't an aggregate, needs to be listed in the GROUP BY. This is done so that data isn't lost from grouping things that might not be equal together.

I'm not sure if would be entirely possible, but I think the only real way to fix this would be to look closer at how the search module does keyword searching now in D7, since it would be working across all DBs, and see if that can be implemented for views.

salbertz’s picture

You're right, thanks for the correction. You may be right about mysql as well...

Ok, so the $group_by is correct in strict SQL. Merely from the perspective of views' search mechanism (which in this case seems to be mysql-centric itself!) it is incorrect - since that mechanism exactly wants to accomplish, what PostgreSQL seems to prohibit, namely to aggregate by count(*) at any price (even at the price that, in your words, distinctive "data is lost") - to have the condition count(*) >= $num_search_terms met.

Then the only proper solution would indeed be a totally alternative approach, by reformulating the search condition (in views/modules/search/views_handler_filter_search.inc - I hope it can be done there, without involving core search issues).

salbertz’s picture

I think I got it now. We were on the wrong track all the time - thanks to r_smylski for clarifying that (and reminding us of the dangers of MySQL-centrism ;-)

It seems, not all the additional GROUP BYs added by views_plugin_query_default::query() (that PostgreSQL needs) are detrimental to the search condition. It seems to be mainly the fact that "score" is added to $non_aggregates, that spoiles the HAVING-clause.

views/modules/search/views_handler_filter_search.inc seems to be absolutely correct in this respect: it accurately adds the information that "score" is an aggregate function in line 134:

$this->search_score = $this->query->add_field('', "SUM($search_index.score * $search_total.count)", 'score', array('aggregate' => TRUE));

What seems to go wrong, is that this information is utterly ignored by views_plugin_query_default::compile_fields() (in line 1208) and the field is added to $non-aggregates indifferently, as soon as it is found to be a function (i.e. to have no table):

// This is a formula, using no tables.
elseif (empty($field['table'])) {
   $non_aggregates[] = $fieldname;
   ...

If I change the code to check whether the field was marked as "aggregate" beforehand, everything seems to work fine:

elseif (empty($field['table'])) {
   if (empty($field['aggregate']) || $field['aggregate'] != true) {
      $non_aggregates[] = $fieldname;
   }

(Sorry, no experience with providing and testing patches so far. Anyway, maybe someone could verify that this could be an alternative solution, and that it works for other practical cases as well.)

mxh’s picture

Status: Needs review » Needs work

Changing to proper status, assuming #13 won't provide a global compatible solution.

dmegatool’s picture

#24 fixed it for me. I had nodes listed multiple times when using more than 1 word. Now it works as it should...

Anonymous’s picture

Just tested the fix from #24, works for me too.

Anonymous’s picture

Taking a stab at creating a patch for the fix. Here it is.

Anonymous’s picture

Status: Needs work » Needs review
Anonymous’s picture

After looking over that suggested code, I don't believe it to be the best course of action. The problem isn't with views_plugin_query_default.inc, it is actually with how add_field() is being called from views_handler_filter_search.inc. There is code to catch if a field is actually a function, but in this case, it isn't being set up as one, and that's why there is the issue.

I've create a new patch, that adds the appropriate info to add 'score' as a field that is a function properly, and works without the change to views_plugin_query_default.inc.

There are 2 files in which the improper call was made, so the patch addresses both.

salbertz’s picture

I would prefer the patch #28 above #30 for several reasons:

1) Not every function is an aggregate function. To my understanding the crucial distinction to decide whether a field should be added to $non_aggregates or not, should primarily depend on the 'aggregate'-flag (at least as far as I understand the inherent logic of the design - what else should that flag be good for?) and not on the assumption that every field containing a function will declare it explicitly.

2) patch #30 could - theoretically at least - have side effects for custom modifications via hooks in applications (if these modifications depend on the old field declaration). For example in my own application I wanted to "blur" the score a bit, to enable the secondary 'ORDER BY'-field to take over sort precedence if the score of two items is similar. To achieve that I modified the query in hook_views_query_alter():

        $view->query->fields['score'] = array(
          'field' => 'ROUND(SUM(search_index.score * search_total.count) * 100)',
          'alias' => 'score',
          'aggregate' => true,
       );

Whether that is very elegant or not - this simple approach is a somewhat natural thing to do, if one wants to modify the sort order. Anyone who has done similar things will not benefit from the fix (although in this case the change would not break anything), until he also adds the explicit 'function'-declaration:

        $view->query->fields['score'] = array(
          'field' => 'SUM(search_index.score * search_total.count * 100)',
          'alias' => 'score',
          'aggregate' => true,
          'function' => 'ROUND',
       );

3) This should also illustrate that the resulting logic is - at least - somewhat counter-intuitve. In order to prevent my field from being added to $non_aggregates and keep my custom scoring function, I have to declare the outer function 'ROUND' (which not even is an aggregate function), while the explicit setting "'aggregate' => true" is de facto irrelevant (and could have been omitted without making any difference). Anyone who does not know about the 'implicit logic' (and has not examined the source code) will fail if he tries to provide an aggregate function by simply writing it down in the "field"-attribute (like the previous author of the original code also did).

Even if the "function"-declaration may be better practice - according to the original logic, the above 'field-only' approach should be permitted as well - at least this is how I would interpret the comment in the (if this assumption is correct, still faulty!) code in compile_fields()

   // This is a formula, using no tables.
   elseif (empty($field['table'])) {
       $non_aggregates[] = $fieldname;
       ....

4) Last not least, the above location is where the bug became manifest: the line '$non_aggregates[] = $fieldname;' was apparently added in views-3.4 (maybe in conjunction with the "pure distinct" feature - probably not without reason, but unfortunately without any comment). But that this elseif-branch on the one hand seems to be explicitly intended for "formulas", but on the other ignores the aggregate-flag completely, still seems like an error to me.

So, if anything, than IMHO both patches should be combined. The fix #28 is "minimally invasive" (I can not imagine any side effects, besides fixing the bug introduced with 3.4) and will guarantee that queries altered via hook will work as expected, whereas the code in #30 (alone) will not help (or might even cause minor trouble) for applications with customized queries like in the example above - but may be cleaner from a general view and thus provide a better base for the future.

There might still be a "better" solution by examining and perhaps rewriting some of the 'if-else'-logic in views_plugin_query_default::compile_fields() or adding comments why something is done in this or that order, as some of the comments suggest that the section needs cleanup and/or commenting. But in the short term at least patch #28 should (and IMHO could without any dangers) be committed to fix the bug for now.

gianfrasoft’s picture

Patch at #28 solved my problem.

Thank you everybody.

codesidekick’s picture

If you don't feel like patching or hacking views then there's another way around this issue ...

/**
  * Implements hook_views_query_alter().
  */
function my_module_views_query_alter(&$view, &$query) {
  if ($view->name == 'MY_SEARCH_VIEW') {
    $query->fields['score']['table'] = 'foo';
  }
}

Ok so there's no table 'foo' - and it's pretty disgusting (I'm probably on merlinofchaos's hit list now) - but from what I can see, because the score field is already an aggregate, views ignores the table field anyway - however because the table field exists, the group by loop respects it as an aggregate field and ignores it in the grouping.

When I perform this query alter, and analyze the views query output there's no mention of the table 'foo' anywhere - so there shouldn't be any SQL errors.

Stefan Lehmann’s picture

I must confess, that I didn't read the whole thread, but I experienced the same problem and I had to do something like this here to show any results, differing from the solution in #33:

/**
  * Implements hook_views_query_alter().
  */
function my_module_views_query_alter(&$view, &$query) {
  if (isset($query->fields['score'])) {
    unset($query->fields['score']);
  }
}
illeace’s picture

Thanks Stefan. This workaround (#34) was just what I needed until a full fix makes its way into core.

timonweb’s picture

Stefan, thank you very much, this saved me some headache!

codesidekick’s picture

Can you still sort by search relevancy using the solution post in 33? I think if you unset score it completely removes it from the view but I might be wrong.

Neil C Smith’s picture

@interactivejunky From your comment I think you're actually referring to #34 (and not your own post!), which you're right doesn't allow the sort by score. I'm doing a mix of #33 & #34 (as below) so as not to need to use the view name. Sorting still seems to work fine then. However, I noticed you can't display the score field doing this - displaying it always shows up as 0.

<?php
<?php
/**
  * Implements hook_views_query_alter().
  */
function my_module_views_query_alter(&$view, &$query) {
  if (isset($query->fields['score'])) {
    $query->fields['score']['table'] = 'foo';
  }
}
?>
?>
mxh’s picture

So is patch from #30 a good one or not? Does it work with all databases? Can it be committed?

I think it's time to get this issue solved.
r_smylski, thanks for your patch.

JonMcL’s picture

As a temporary hack, NOT A SOLUTION, and an alternative to #34:

function my_module_query_alter(QueryAlterableInterface $query) {
  if (isset($query->alterTags['views']) && isset($query->alterTags['views_search_view_id'])) {
    //Get a list of all 'group by' in the query
    $fields =& $query->getGroupBy();
    unset($fields['score']);
  }
}

This just removes score from the GROUP BY part of the SQL but leaves it in the SELECT. I guess this won't work for PostgreSQL. (I'm just getting up to speed on this issue)

Adapt the IF statement and set 'views_search_view_id' to the correct ID for your view.

salbertz’s picture

I can still only affirm what I wrote in #24: 'score' simply must not be added to the 'GROUP BY' clause, but views_plugin_query_default::compile_fields() does exactly that since views-3.4. And that's an error that should be corrected, period. Which is very simple to do and which is exactly what r_smylski's patch #28 does (just tested it again with views-3.6, still works great).

Patch #30 works as well and is considered 'more correct' by r_smylski, while I hold the opposite view (for several reasons pointed out in #31). #28 depends on the field attribute 'aggregate', #30 on 'function' - so it seems to be a matter of personal preference. In my view #28 simply corrects the error introduced in 3.4 (after all, everything worked fine before that, didn't it?) and therefore should be considered the most natural and least problematic approach.

But of cause either solution is better than messing around with hook_views_query_alter and discarding the score functionality (including sorting) altogether – that can't be a serious option.

Anonymous’s picture

I'm good with the patch from #28 being used, as it fixes the changed functionality, and I'd love to see this finally put to bed. Plus, it would catch any other instances of the same usage by any other plugin or contrib that might use it as a basis for its functionality.

yannickoo’s picture

Works fine for me.

greggles’s picture

I'm not sure if #28 is a good solution, but I can also confirm I had the problems described here and #28 fixed it for me.

mxh’s picture

#34 is working for me as well, I'm using this workaround only on my view which is actually using search terms:

<?php
function my_module_views_query_alter(&$view, &$query) {
  if ($view->name == 'my_search_view' && isset($query->fields['score'])) {
    unset($query->fields['score']);
  }
}
?>

Thanks for your help, guys. I dont' know if this is a good solution for a fix in the module, since this maybe can break sites using PostgreSQL or other dbms. But at least my nodes appear now.

mpp’s picture

When updating to views-3.4 we had a case where aggregate were used which caused a faulty query (too many group by's).
#28 was a good fix

mxh’s picture

I think we definetly need a Views maintainer who really knows what he's doing on this problem. Both #28 and #34 seem to fix this, but nobody knows exactly why and even worse nobody following this issue knows if these fixes could make other problems then. I hope a maintainer will look over this soon.

Rob230’s picture

Just ran into this on a completely new site. Searching for 1 word will return results. Searching for more than one word returns no results. Seems like quite a major bug! I'm wary about the patches above since nobody seems sure what's going on exactly.

dawehner’s picture

If I had to choose I would be really going with #30 given that it don't interfere with other views. If this workaround fork for people which just have a single argument as well, I think this would be ready to fly.

mxh’s picture

Thanks for your feedback, Daniel. #30 is working for me as well as #28. If you think #30 is the better fix than #28, then let's go for it. But I agree with salbertz in his statement from #31 that there might be new upcoming problems caused by custom extensions which were made between Views 3.4 and the current version. :/

mxh’s picture

Issue summary: View changes

This is a Viess module issue.

salbertz’s picture

I'm still using the solution #24/#28 with the current views version (3.7) and can confirm that it still works great.

As repeated in #41: If you look at the code block cited in #24 (views-3.4 to present) and compare it to earlier versions, you will find that the present logic is simply flawed, while the "old logic" seemed perfectly reasonable. I can't see any reason for the change in 3.4 (and there is no comment to justify it) - the patch simply reverts to the "old logic", which still seems intuitive and correct - and worked perfectly.

If the problematic change introduced in 3.4 was really that vital to anything, the author should at least have left a comment, what it should be good for. Given the absence of such a hint, I still consider it some unintentional (or unfortunate - maybe the author wanted to 'clean up' a little bit... everyone of us knows that urge and its pitfalls, especially in conjunction with complex conditional branching) 'collateral damage'.

drupov’s picture

I tried both #28 and #30 and both seem to work pretty well.

I am going with #28 now, will report if I experience something strange.

Thanks!

n3or’s picture

Version: 7.x-3.x-dev » 7.x-3.7

Problem still exists and patch in comment #30 still works!

n3or’s picture

Status: Needs review » Needs work

The last submitted patch, 30: search_multiple_terms-1615438-30.patch, failed testing.

The last submitted patch, 30: search_multiple_terms-1615438-30.patch, failed testing.

greggles’s picture

Status: Needs work » Needs review
salbertz’s picture

Again: Please consider the patch #28 instead; as I stated in #51 and #41 it should be the better, simpler and safer solution, simply correcting an obvious mistake introduced in views-3.4.

RoloDMonkey’s picture

happysnowmantech’s picture

I tested patch #30 and it worked for me.

magicmyth’s picture

Status: Needs review » Reviewed & tested by the community

Like many here. I've been using #28 on a few sites for a long time now and everything has continued to work as expected. To me it does seem the safer route to take as it deals specifically with the cause of this problem. However what is being tackled in #30 should probably be expanded upon. As #1615438-30: Search terms: nodes dont appear when using multiple arguments mentions. views_handler_filter_search.inc is not handling add_field() correctly. I think another issue that deals specifically with that potential problem should be opened where some longer term testing can be done to try and catch any potential fall out. Could do with a notice going out to all modules that depend on views to look at and test the solution to that issue (though how many modules do not depend on Views these days? o_0).

As it stands now the debate over the two solutions is holding up fixing a quite critical issue that was introduced by a View's release (3.4 I believe) and #28 does deal with that specifically.

adr75’s picture

We ran into the same issue of not being able to use multiple search terms in a views search (using Views 7.x-3.8). #28 appears to be working.

The last submitted patch, 28: search_multiple_terms-1615438-28.patch, failed testing.

mxh’s picture

Version: 7.x-3.7 » 7.x-3.x-dev
Status: Reviewed & tested by the community » Needs review

Apparently its not yet commited. Therefore its a case for dev.
Also it should be reviewed again with the current dev version.

ginosuave’s picture

I tried #28 and got an error: "Patch does not apply". The lines match up, so I guess it's a whitespace or OS issue (I'm on a Windows server).

#30 works though. It would be great if this got resolved in the next release since we're running a distribution and don't want to keep maintaining this.

pendaco’s picture

Why hasn't this been resolved yet? C'mon guys get this fixed!

I'm on views 3.8 and this issue is still present! Will see if #28 or #30 works for now..

hargobind’s picture

I just tested both patches and they apply cleanly to 7.x-3.x-dev and 7.x-3.10. I cannot figure out how to test the patches, but at least they don't break existing search functionality as far as I could tell.

salbertz’s picture

Just applied #28 to views 7.x-3.10 without problems (for linux servers at least) and it still works great. I agree that this is a major bug which should be fixed in the near future; and #28 is an extremely simple and transparent way to fix precisely what was going wrong since 3.4. The question raised by #30 about further inconsistencies may be absolutely legitimate, but probably should be adressed elsewhere, since they are not directly responsible for this particular bug.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

As other people have commented, the patch in #28 has very wide consequences and will also most likely break PostgreSQL.

The patch in #30, on the other hand, fixes this problem and is limited to fixing just the actual problem, in the Search Terms filter and argument.

The patch in #30 does fix this problem, and it also fixes #1677692: Remove duplicates from exposed search filter results (I'll comment on that issue shortly). As a sometimes-maintainer of the core Search module, I think it's the right fix. I'd recommend committing it.

salbertz’s picture

I can't see why #28 should break PostgreSQL - those comments refer to other solutions, if you read the conversation carefully. In fact, rsmylki rightfully pointed out possible problems with PostgreSQL in solutions _prior_ to #24 - and in response provided #28, adhering strictly to Standard SQL - precisely _not_ to break PostgreSQL!

But if you insist on #30 - and if other issues benifit from that solution as well - go ahead, as long as it will finally be fixed. My point (and that of several others, e.g. #61) was that I judged #28 to lack any 'wide consequences' and therefore to be _less_ prone to possible side effects than #30, since it simply restores the state of affaires prior to 3.4, when everthing still worked fine - even with PostgreSQL, I dare guess. (And for me it still seems more straightforward and easier to understand, since the way the attribute "aggregate" ist handled in solution #30 (see my argument #31) does not seem very transparent to me.)

But, as said, if all the experts consider #30 to be ok, I will be fine with it as well.

hanoii’s picture

#30 does work, haven't really tested #28 though.

tko’s picture

#30 works.

Anonymous’s picture

I'm still on board with @salbertz in that the patch from #28 is the 'best' patch to get committed as soon as possible. It is essentially what views had been doing in versions prior to 3.4, and nobody can give any reason why it was changed in the first place.

#30 does fix this problem, but needs very thorough testing as it is a big change to existing functionality. Although it makes the function work as it should actually work, the fallout for other code relying on it not working properly, but as it has, is very unknown.

mxh’s picture

Didn't have any problems both with #28 and #30. Since dawehner and jhodgdon would go for #30 and no problems were reported for #30 in this long time, I'd say yes, please commit #30.

Kristi Wachter’s picture

A temporary workaround:

In the Configure Filter Criterion pane for Search: Search Terms, select the Remove search score option. (Under Filter criteria, click Search: Search Terms; in the popup, click the Remove search score checkbox so it's checked.)

In my testing, that solves the problem (for views that don't need to use the search score, obviously).

This might help those who aren't sure which patch to apply, and may also be useful to the module's developers in pinning down the best approach to solving this problem in code.

mxh’s picture

Thanks for the hint Kristi. You're right, this works when you use search terms as exposed filter, but unfortunately this option doesn't seem to be available for the contextual filter.

BUT, your tip has brought me to an even more elegant solution than I have done before :)

If you already use the power-trio CTools (Page Manager), Panels and Views, you could create a view display of type Panel pane, configure the view as you like - but without the use of the contextual filter. Just use the filter for search terms, expose it and check to disable search score. At the Panel: Allow Settings configuration, select "Use exposed widgets form as pane configuration". This makes it possible to use the exposed filter through Panels.
On the CTools Page Manager UI, activate "search-node", edit this page, add a panel variant. Embed the panel view you just created and use %keywords:html_safe as parameter for the exposed search terms filter. You also can add the search form as Ctools widget. For me, this is a fine solution.

MediaFormat’s picture

Rerolling patches #28, #30 based on current dev

Preference by author of both patches (rsmylski) for #28

Lets commit this already!

GiorgosK’s picture

+1
should commit 79 (30) patch

colan’s picture

We've recently switched our testing from the old qa.drupal.org to DrupalCI. Because of a bug in the new system, #2623840: Views (D7) patches not being tested, older patches must be re-uploaded. On re-uploading the patch, please set the status to "Needs Review" so that the test bot will add it to its queue.

If all tests pass, change the Status back to "Reviewed & tested by the community". We'll most likely commit the patch immediately without having to go through another round of peer review.

We apologize for the trouble, and appreciate your patience.

ParisLiakos’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
2.22 KB

reuploading search_multiple_terms-1615438-79(30).patch

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

back to rtbc

  • colan committed 7904b94 on 7.x-3.x authored by ParisLiakos
    Issue #1615438 by rsmylski, MediaFormat, awolfey, ParisLiakos: Search...

  • colan committed 41fbac7 on 7.x-3.x authored by rsmylski
    Issue #1615438 by rsmylski, MediaFormat, awolfey, ParisLiakos: Search...
  • colan committed 6800d15 on 7.x-3.x
    Revert "Issue #1615438 by rsmylski, MediaFormat, awolfey, ParisLiakos:...
colan’s picture

Project: Views (for Drupal 7) » Drupal core
Version: 7.x-3.x-dev » 8.0.x-dev
Component: search data » views.module
Status: Reviewed & tested by the community » Patch (to be ported)

Thanks! Sorry, wrong attribution the first time.

xjm’s picture

Version: 8.0.x-dev » 8.1.x-dev
Status: Patch (to be ported) » Postponed (maintainer needs more info)

Can we test and confirm that this bug actually also applies to D8 core? Thanks!

jhodgdon’s picture

Component: views.module » search.module

Also in 8.x it would be search.module... but really we should have a separate issue for core if it is an issue in 8.x.

So, can someone test this in 8, or provide clear steps to reproduce? And if so, set this back to a Views 7.x issue, and make a new issue for Drupal Core version 8. Thanks!

phjou’s picture

The patch #30 corrects this issue for me on Drupal 7. Thanks.

jhodgdon’s picture

Project: Drupal core » Views (for Drupal 7)
Version: 8.1.x-dev » 7.x-3.x-dev
Component: search.module » Code
Status: Postponed (maintainer needs more info) » Fixed

I just tested this on Drupal 8.2.x. What I did:

a) Installed with Standard profile in English.

b) Created a Basic Page node with title "This is the title", and body "This is the body".

c) Ran cron so this is indexed for search.

d) Using the standard Search box, verified if I search for "title body" (without the quotes), I get the node as a search result.

e) Created a page view. Set it up with:
- Published content
- Search keywords exposed filter

f) Went to the page. Tried the following keywords in the search keywords exposed filter box:
- title => found it
- body => found it
- spam => didn't find it
- title body => found it

So... This is not a bug in Drupal 8. Setting this back to Views module, Drupal 7. And since the previous patch was committed, I think this is Fixed.

Status: Fixed » Closed (fixed)

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

John Pitcairn’s picture

Status: Closed (fixed) » Reviewed & tested by the community

Back to RTBC for Drupal 7?

jhodgdon’s picture

Status: Reviewed & tested by the community » Closed (fixed)

The Drupal 7 Views patch was already committed.

John Pitcairn’s picture

Oops, apologies.