bzrudi71 on #2158339-8: Search results page broken on PostgreSQL reported that for Postgres, advanced search does not work.

I just tested in MySQL and it also doesn't work there. Here are the steps to reproduce:

- Clean 8.x install with the Standard install profile.
- Enable Content Translation and Language modules.
- Install a language. I used Spanish.
- Enable translation for the Article content type, and make sure to check the "show language choices on form" box or whatever it's called there. Then go to Manage Fields and edit the settings for Body to make it a translatable field.
- Create a few nodes in English and Spanish. I also made a translation of one of them. Make sure that all nodes and translations in both languages include one shared keyword.
- Run cron to update the search index.
- Go to search/node. Type in your shared keyword. Attempt to filter the search by language. It is not working right.

So in my particular case, I made one just-English node (1), one that has both English and Spanish translations (2), and one just Spanish (3).

When I search for my shared keyword without advanced filters, I end up with the English versions of 1 & 2 and the Spanish 3 shown in the results. The same thing happens no matter what language filters I select. When I filter to just the "Page" content type, that works (none of my three Articles shows up).

CommentFileSizeAuthor
#29 interdiff-2161067-23-29.txt1.64 KBchakrapani
#29 advanced_Search_language_filter-2161067-29_PASS.patch7.72 KBchakrapani
#29 advanced_Search_language_filter-2161067-29_FAIL.patch6.41 KBchakrapani
#23 interdiff-2161067_18-23.txt3.91 KBchakrapani
#23 advanced_Search_language_filter-2161067-23_PASS.patch7.15 KBchakrapani
#23 advanced_Search_language_filter-2161067-23_FAIL.patch5.84 KBchakrapani
#18 interdiff-2161067_11-18.txt1.29 KBchakrapani
#18 advanced_Search_language_filter-2161067-18_PASS.patch3.1 KBchakrapani
#18 advanced_Search_language_filter-2161067-18_FAIL.patch1.78 KBchakrapani
#11 2161067-11.patch2.24 KBnick_vh
#8 2161067-8.patch2.24 KBnick_vh
#3 2161067-1.patch1.32 KBnick_vh

Comments

nick_vh’s picture

nick_vh
he/him
Location Ghent
commented

Bug confirmed, steps to reproduce this are sound and clear. Trying to take a crack at this now.

nick_vh’s picture

nick_vh
he/him
Location Ghent
commented
Issue tags: +#SprintWeekend2014, +SprintWeekend2014
nick_vh’s picture

nick_vh
he/him
Location Ghent
commented
Status: Active » Needs review
StatusFileSize
new 2161067-1.patch1.32 KB

Found the missing part. The entry as it comes from advanced search is language and not langcode. Changing this makes the behavior work as expected.

nick_vh’s picture

nick_vh
he/him
Location Ghent
commented

Also updated a deprecated function while I was at it. Let's see what the testbot says.

Status: Needs review » Needs work

The last submitted patch, 3: 2161067-1.patch, failed testing.

The last submitted patch, 3: 2161067-1.patch, failed testing.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

Thanks!

We also need a test for this, and one existing test fails with this change.

nick_vh’s picture

nick_vh
he/him
Location Ghent
commented
StatusFileSize
new 2161067-8.patch2.24 KB

Should fix the test. It was basically sending the langcode as url param while it needed to be language as that is what drupal sends to the URL

nick_vh’s picture

nick_vh
he/him
Location Ghent
commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 8: 2161067-8.patch, failed testing.

nick_vh’s picture

nick_vh
he/him
Location Ghent
commented
StatusFileSize
new 2161067-11.patch2.24 KB
nick_vh’s picture

nick_vh
he/him
Location Ghent
commented
Status: Needs work » Needs review

This should at least apply and succeed

Status: Needs review » Needs work

The last submitted patch, 11: 2161067-11.patch, failed testing.

nick_vh’s picture

nick_vh
he/him
Location Ghent
commented
Status: Needs work » Needs review

11: 2161067-11.patch queued for re-testing.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

I haven't tested this patch manually yet, but we also need a test written -- the existing tests did not uncover the fact that the language filter was not working.

So we need to add a new test, demonstrate that it fails without this patch (by uploading a test-only patch), and then demonstrate that it passes with this patch (by uploading a patch with the code changes as well as the new test).

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Needs work
Issue tags: +Needs tests

Fixing status as per last comment. This definitely needs a test.

chakrapani’s picture

chakrapani commented
Assigned: Unassigned » chakrapani

Looking into this..

chakrapani’s picture

chakrapani commented
Assigned: chakrapani » Unassigned
Status: Needs work » Needs review
StatusFileSize
new advanced_Search_language_filter-2161067-18_FAIL.patch1.78 KB
new advanced_Search_language_filter-2161067-18_PASS.patch3.1 KB
new interdiff-2161067_11-18.txt1.29 KB

The earlier patch #11 does have a change to a test which fails without the fix. Anyways I've added additional tests to check for the search with common keyword in multiple languages. Also checks for no. of results with language filter and checks language of the resulted item.

inter-diff contains the changes from #11 to the PASS patch.
the FAIL patch should FAIL which does not have the fix to the issue.
Lets see what the test-bot has to say.

The last submitted patch, 18: advanced_Search_language_filter-2161067-18_FAIL.patch, failed testing.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Issue tags: -#SprintWeekend2014 +D8MI, +multilingual

OK, good job!

So I think we need to step back a moment and think about what the correct result should actually be.

In SearchMultilingualEntityTest, the setUp() creates three English nodes, and it adds a Hungarian "translation" to the second node, and both a Hungarian and 'sv' "translation" to the third node. (For testing purposes, the "translations" are not really in foreign language text -- all 6 of the translations have the word "node" in their title, and randomly-generated body text.)

Anyway, there are only three nodes, but a total of 6 node translations.

So, this test checks that with no language filtering, if we search for the word "node", we get 6 results. I'm just not sure that is what we want, and I think the current behavior without this patch is that we would only get the three base nodes, not multiple translations of the same node. I am just not sure what behavior should be considered "correct" here. We should probably consult with the Multilingual team to figure this out before we decide the issue has been fixed... adding what I hope are correct tags so they'll notice this, and we could also ping people in IRC (but I have to run out now so will check later).

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Needs work

Upon further thought, I think the behavior is correct. The old behavior originally reported in this issue was incorrect, and it was corrected on another issue.

But, we do need another test. The test here is good, but it does not test the UI. We need a test that goes to search/node, chooses a language, submits the form, and verifies that the correct nodes are shown on the page (looking for links to the titles is a good way to do that). A good place to add that might be in SearchLanguageTest, which purports to test language searching but actually doesn't ever check the search results. We'd need to add nodes similar to the test in this patch, but with French, and make sure they're indexed.

chakrapani’s picture

chakrapani commented
Assigned: Unassigned » chakrapani
chakrapani’s picture

chakrapani commented
Assigned: chakrapani » Unassigned
Status: Needs work » Needs review
StatusFileSize
new advanced_Search_language_filter-2161067-23_FAIL.patch5.84 KB
new advanced_Search_language_filter-2161067-23_PASS.patch7.15 KB
new interdiff-2161067_18-23.txt3.91 KB

Here is the updated patch with additional tests based on suggestion in #21.
interdiff contains changes and it is same for PASS and FAIL patches.
Retained the tests in #18.
Added additional equivalent tests which actually tests UI which include the following:

  • Add new language Spanish(code=es). (Din't use French as it was already used to do other tests)
  • Add 3 nodes(2 english and 1 spanish)
  • Added translations(one english to spanish and one spanish to english)
  • navigate to search/node
  • Submit advanced search form with common keyword node and language filter Spanish.
  • Check for existence of Spanish node titles/links in the search results.
  • Ensure that there are no other(language) search results.

The last submitted patch, 23: advanced_Search_language_filter-2161067-23_FAIL.patch, failed testing.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

Excellent! I think this is now ready to go in. I could quibble a bit with grammar in some of the test assert messages, but I think everything is quite readable, so I think we should just get it in.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/modules/search/lib/Drupal/search/Tests/SearchLanguageTest.php
@@ -33,6 +35,62 @@ function setUp() {
+    $field = field_info_field('node', 'body');

This is deprecated and will be removed soon, see https://api.drupal.org/api/drupal/core%21modules%21field%21field.depreca...

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

OK. So it looks like this test that calls field_info_field() instead needs to have

use Drupal\field\Field;

at the top of the file, and then it needs to replace field_info_field(...) with:

Field::fieldInfo()->getField(...)

chakrapani: would you like to make a new patch? It looks like we should make this change both in the existing SearchMultilingualEntityTest and the new SearchLanguageTest.

Thanks!

chakrapani’s picture

chakrapani commented
Assigned: Unassigned » chakrapani

@catch, thanks for the catch :)

@jhodgdon, Yes I will add a new patch.

chakrapani’s picture

chakrapani commented
Assigned: chakrapani » Unassigned
Status: Needs work » Needs review
StatusFileSize
new advanced_Search_language_filter-2161067-29_FAIL.patch6.41 KB
new advanced_Search_language_filter-2161067-29_PASS.patch7.72 KB
new interdiff-2161067-23-29.txt1.64 KB

Here we go..Added patches with the suggested change in #26 and #27.
interdiff is same for both FAIL and PASS patch.

The last submitted patch, 29: advanced_Search_language_filter-2161067-29_FAIL.patch, failed testing.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

Thanks, looks right to me, and the tests are passing/failing as they should. Back to RTBC. One less deprecated function in the code base. :)

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed 
+++ b/core/modules/search/lib/Drupal/search/Tests/SearchMultilingualEntityTest.php
@@ -133,10 +134,27 @@ function testSearchingMultilingualFieldValues() {
+    // Test with language filters and common key word

Periods missing from both these comments but I fixed locally and committed/pushed to 8.x, thanks!

Status: Fixed » Closed (fixed)

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