Problem/Motivation

I'm working on date facets for the core search.When clicking on a date filter basically we need to alter the query to add that condition, this is how it works.

Ok so the problem is when we have a multilingual site. In this case we add the filter but we have the same nid in 2 different languages:

INNER JOIN node_field_data n ON n.nid = i.sid
INNER JOIN search_total t ON i.word = t.word
INNER JOIN search_dataset d ON i.sid = d.sid AND i.type = d.type AND i.langcode = d.langcode
WHERE (n.status = 1) AND( (i.word = "test") )AND (i.type = "node_search") AND (n.created >= 1458397200) AND (n.created < 1458397260) AND( (d.data LIKE "% test %" ESCAPE '\\') )

Here is the result of a screeshot:

The date is different but the nid is the same.

Proposed resolution

We need to verify that the index langcode is the same for the node_field_data results.

So should be something like this:

INNER JOIN node_field_data n ON n.nid = i.sid AND n.langcode = i.langcode
INNER JOIN search_total t ON i.word = t.word
INNER JOIN search_dataset d ON i.sid = d.sid AND i.type = d.type AND i.langcode = d.langcode
WHERE (n.status = 1) AND( (i.word = "test") )AND (i.type = "node_search") AND (n.created >= 1458397200) AND (n.created < 1458397260) AND( (d.data LIKE "% test %" ESCAPE '\\') )

And this is the interesting part:
INNER JOIN node_field_data n ON n.nid = i.sid AND n.langcode = i.langcode

And here is the result applying the attached patch:

CommentFileSizeAuthor
#11 interdiff-2694243-9-11.txt1.45 KBmarthinal
#11 2694243-11.patch4.85 KBmarthinal
#9 interdiff-2694243-6-9.txt7.65 KBmarthinal
#9 2694243-9.patch4.79 KBmarthinal
#6 2694243-6.patch6.53 KBmarthinal
#6 2694243-6-only-test-should-fail.patch5.78 KBmarthinal
#5 2694243-5.patch2.2 KBmarthinal
#5 2694243-5-only-test-should-fail.patch1.45 KBmarthinal
core_facet.patch768 bytesmarthinal
multilingual_date-2.png125.49 KBmarthinal
multilingual_date-1.png140.01 KBmarthinal
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

marthinal created an issue. See original summary.

marthinal’s picture

marthinal CreditAttribution: marthinal commented
Issue summary: View changes
jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -node search +language content, +D8MI

That definitely looks correct to me. Thanks for the patch!

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

Although actually we probably need a failing test for this bug before this patch will be accepted into Core.

marthinal’s picture

marthinal CreditAttribution: marthinal commented
Status: Needs work » Needs review
FileSize
2694243-5-only-test-should-fail.patch1.45 KB
2694243-5.patch2.2 KB

Adding the integration test to reproduce the bug.

@jhodgdon Thanks for reviewing!

marthinal’s picture

marthinal CreditAttribution: marthinal commented
FileSize
2694243-6-only-test-should-fail.patch5.78 KB
2694243-6.patch6.53 KB

Oops, sorry I forgot the test

The last submitted patch, 6: 2694243-6-only-test-should-fail.patch, failed testing.

jhodgdon’s picture

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

Looks good! The added test seems to be failing in the right way, and I like that you used a test case that mirrored your actual use case that uncovered the problem.

I only have a few documentation nitpicks, and then I think we can get this in:

  1. +++ b/core/modules/search/src/Tests/SearchDateIntervalTest.php
@@ -0,0 +1,116 @@
+
+/**
+ * @file
+ * Contains \Drupal\search\Tests\SearchDateIntervalTest.
+ */

    I don't think we're putting @file doc blocks in Core any more for files containing exactly one PSR-4 class. There was a recent issue... 

  2. +++ b/core/modules/search/src/Tests/SearchDateIntervalTest.php
@@ -0,0 +1,116 @@
+ * Tests interval dates conditions with different languages added.

    Hm. This is a bit odd..

    How about:

    Tests searching with date filters that exclude some translations. 

  3. +++ b/core/modules/search/src/Tests/SearchDateIntervalTest.php
@@ -0,0 +1,116 @@
+   * @var array

    should be @var string[] 

  4. +++ b/core/modules/search/src/Tests/SearchDateIntervalTest.php
@@ -0,0 +1,116 @@
+  public static $modules = array('language', 'search_date_query_alter');

    I think we also need 'locale' here? 

  5. +++ b/core/modules/search/src/Tests/SearchDateIntervalTest.php
@@ -0,0 +1,116 @@
+    // Create and login user.

    login => log in

    (when it's a verb, it's two words) 

  6. +++ b/core/modules/search/src/Tests/SearchDateIntervalTest.php
@@ -0,0 +1,116 @@
+    // Make the body field translatable. The title is already translatable by
+    // definition. The parent class has already created the article and page
+    // content types.
+    $field_storage = FieldStorageConfig::loadByName('node', 'body');
+    $field_storage->setTranslatable(TRUE);
+    $field_storage->save();

    Where is the node content type itself set to be translatable? This seems to be missing something... 

  7. +++ b/core/modules/search/src/Tests/SearchDateIntervalTest.php
@@ -0,0 +1,116 @@
+    // Adding a different created time per language to avoid to have exactly
+    // the same value per nid and langcode.

    Suggested comment text here:

    Set up times to be applied to the English and Spanish translations of the node create time, so that they are filtered in/out in the search_date_query_alter test module. 

  8. +++ b/core/modules/search/src/Tests/SearchDateIntervalTest.php
@@ -0,0 +1,116 @@
+    // Create a few page nodes with multilingual body values.

    I don't really understand why you need more than one node here. As far as I can tell, only the third one is actually used in the test. 

  9. +++ b/core/modules/search/src/Tests/SearchDateIntervalTest.php
@@ -0,0 +1,116 @@
+   * Tests interval dates adding conditions from query alter.

    How about:

    Tests searching with date filters that exclude some translations. 

  10. +++ b/core/modules/search/src/Tests/SearchDateIntervalTest.php
@@ -0,0 +1,116 @@
+    $this->drupalPostForm('search/node', $edit, t('Advanced search'));

    This isn't really an advanced search I think? 

  11. +++ b/core/modules/search/src/Tests/SearchDateIntervalTest.php
@@ -0,0 +1,116 @@
+    // Third nodes must have the same nid but the created date is different. So

    Do not use "nid" in comments. Say "node ID" instead. 

  12. +++ b/core/modules/search/src/Tests/SearchDateIntervalTest.php
@@ -0,0 +1,116 @@
+    // only the spanish translation must appear. @See search_date_query_alter

    spanish -> Spanish

    Also ... We don't normally put @see in // comments. And it's lower-case if you did. 

  13. +++ b/core/modules/search/tests/modules/search_date_query_alter/search_date_query_alter.info.yml
@@ -0,0 +1,6 @@
+name: 'Test to alter the query adding date conditions to the node search.'
+type: module
+description: 'Search Date Query Alter.'

    I think you have the module name and description reversed.

    Also maybe a better description would be:

    Test module that adds date conditions to node searches. 

  14. +++ b/core/modules/search/tests/modules/search_date_query_alter/search_date_query_alter.module
@@ -0,0 +1,20 @@
+ * Contains search_pager_test.module

    This must have been copied from another module? 

  15. +++ b/core/modules/search/tests/modules/search_date_query_alter/search_date_query_alter.module
@@ -0,0 +1,20 @@
+ * @param \Drupal\Core\Database\Query\AlterableInterface $query
+ */

    This @param is missing its description... but actually in hook implementations we normally don't include @param at all. 

  16. +++ b/core/modules/search/tests/modules/search_date_query_alter/search_date_query_alter.module
@@ -0,0 +1,20 @@
+  // Start date Sat, 19 Mar 2016 00:00:00 GMT
+  $query->condition('n.created', 1458345600, '>=');
+  // End date Sun, 20 Mar 2016 00:00:00 GMT

    Comments should normally end in .

marthinal’s picture

marthinal CreditAttribution: marthinal commented
Status: Needs work » Needs review
FileSize
2694243-9.patch4.79 KB
interdiff-2694243-6-9.txt7.65 KB

@jhodgdon let's fix these nitpicks! :)

1) Done.

2) Done.

3) Done.

4) SearchLanguageTest is not using Locale module and we have the same situation in this use case. I mean, we only need to add a new language (Spanish) and translate one node.

5) Done.

./core/modules/comment/src/Tests/CommentLanguageTest.php:40:    // Create and login user.
./core/modules/config_translation/src/Tests/ConfigTranslationOverviewTest.php:63:    // Create and login user.
./core/modules/config_translation/src/Tests/ConfigTranslationUiTest.php:112:    // Create and login user.
./core/modules/config_translation/src/Tests/ConfigTranslationUiThemeTest.php:50:    // Create and login user.
./core/modules/file/src/Tests/FileOnTranslatedEntityTest.php:44:    // Create and login user.
./core/modules/file/src/Tests/PrivateFileOnTranslatedEntityTest.php:45:    // Create and login user.
./core/modules/image/src/Tests/ImageOnTranslatedEntityTest.php:44:    // Create and login user.
./core/modules/language/src/Tests/LanguagePathMonolingualTest.php:29:    // Create and login user.
./core/modules/language/src/Tests/LanguageSwitchingTest.php:33:    // Create and login user.
./core/modules/language/src/Tests/LanguageUrlRewritingTest.php:41:    // Create and login user.
./core/modules/node/src/Tests/NodeRevisionsAllTest.php:82:    // Create and login user.
./core/modules/node/src/Tests/NodeTypeTranslationTest.php:66:    // Create and login user.
./core/modules/path/src/Tests/PathLanguageTest.php:46:    // Create and login user.
./core/modules/path/src/Tests/PathLanguageUiTest.php:27:    // Create and login user.
./core/modules/path/src/Tests/PathTaxonomyTermTest.php:36:    // Create and login user.
./core/modules/search/src/Tests/SearchAdvancedSearchFormTest.php:26:    // Create and login user.
./core/modules/search/src/Tests/SearchBlockTest.php:27:    // Create and login user.
./core/modules/search/src/Tests/SearchLanguageTest.php:37:    // Create and login user.
./core/modules/system/src/Tests/Entity/EntityListBuilderTest.php:32:    // Create and login user.
./core/modules/system/src/Tests/Entity/EntityOperationsTest.php:29:    // Create and login user.
./core/modules/system/src/Tests/Entity/EntityRevisionsTest.php:37:    // Create and login user.
./core/modules/views_ui/src/Tests/TranslatedViewTest.php:56:    // Create and login user.

I can create a new issue for these cases if you agree :)

6) Removed.

7) Done.

8) Yep, I agree. Done.

9) Done.

10) Done.

11) Done.

12) Done.

13) Oops Done! :)

14) Removed.

15) Removed.

16) Done.

Thanks!!!

jhodgdon’s picture

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

Thanks!

Regarding item 5, ... Yeah, Core doesn't have very good grammar in comments. But for reference the log in vs. login thing is here in our d.o style guide:
https://www.drupal.org/drupalorg/style-guide/content#relatedwords
and I think that came from the Chicago Manual of Style or some such standard reference. An issue to fix that in comments would be a fine thing.

Regarding item 4, doh! I was thinking language required locale but of course it is the other way around. ;)

So the patch is looking good! One thing though: we can only omit @file for files that contain exactly one class. Not for .module files... so... one more look at the patch as a whole:

  1. +++ b/core/modules/search/src/Tests/SearchDateIntervalTest.php
@@ -0,0 +1,74 @@
+    // The nodes must have the same node Id but the created date is different.

    Id -> ID

  2. +++ b/core/modules/search/src/Tests/SearchDateIntervalTest.php
@@ -0,0 +1,74 @@
+    $this->assertNoLink('Node EN', 'Search results does not contain English node');

    results does not => results do not

  3. +++ b/core/modules/search/tests/modules/search_date_query_alter/search_date_query_alter.module
@@ -0,0 +1,13 @@
+<?php
+
+use Drupal\Core\Database\Query\AlterableInterface;

    This file needs the @file doc block put back in. 

  4. +++ b/core/modules/search/tests/modules/search_date_query_alter/search_date_query_alter.module
@@ -0,0 +1,13 @@
+ * Implements hook_query_TAG_alter(): tag search_$type with $type node_search.

    Nice. Very clear one-line description here. :)

marthinal’s picture

marthinal CreditAttribution: marthinal commented
Status: Needs work » Needs review
FileSize
2694243-11.patch4.85 KB
interdiff-2694243-9-11.txt1.45 KB

Ok! let's try again! :)

1) Done.

2) Hmmm we need to fix this for SearchLanguageTest. Basically I was using this test to create the new one.

    // Ensure that results doesn't contain other language nodes.
    $this->assertNoLink('First node en', 'Search results does not contain first English node');
    $this->assertNoLink('Second node en', 'Search results does not contain second English node');
    $this->assertNoLink('Third node en', 'Search results does not contain third English node');

3) Done.

thanks!!!

jhodgdon’s picture

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

OK, looks perfect now! Thanks!

marthinal’s picture

marthinal CreditAttribution: marthinal commented
Related issues: +#2697909: Fix "login (noun/adjective)" vs. "log in (verb)" in comments (and logout/log out), +#2697933: Replace "does not" with "do not" in SearchLanguageTest.
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Fixed

Committed e9d8f2d and pushed to 8.0.x, 8.1.x and 8.0.x. Thanks!

diff --git a/core/modules/search/src/Tests/SearchDateIntervalTest.php b/core/modules/search/src/Tests/SearchDateIntervalTest.php
index 30f26bd..ec05394 100644
--- a/core/modules/search/src/Tests/SearchDateIntervalTest.php
+++ b/core/modules/search/src/Tests/SearchDateIntervalTest.php
@@ -1,5 +1,10 @@
 <?php
 
+/**
+ * @file
+ * Contains \Drupal\search\Tests\SearchDateIntervalTest.
+ */
+
 namespace Drupal\search\Tests;
 
 use Drupal\language\Entity\ConfigurableLanguage;

Fixed on commit - we still have not applied the new rule to core - so @file doc blocks are mandatory.

  • alexpott committed 552268e on 8.2.x 
    Issue #2694243 by marthinal, jhodgdon: node_field_data and search_index...

  • alexpott committed 120558e on 8.1.x 
    Issue #2694243 by marthinal, jhodgdon: node_field_data and search_index...

  • alexpott committed e9d8f2d on 8.0.x 
    Issue #2694243 by marthinal, jhodgdon: node_field_data and search_index...

Status: Fixed » Closed (fixed)

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