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:
Comment | File | Size | Author |
---|---|---|---|
#11 | interdiff-2694243-9-11.txt | 1.45 KB | marthinal |
#11 | 2694243-11.patch | 4.85 KB | marthinal |
#9 | interdiff-2694243-6-9.txt | 7.65 KB | marthinal |
#9 | 2694243-9.patch | 4.79 KB | marthinal |
#6 | 2694243-6.patch | 6.53 KB | marthinal |
Comments
Comment #2
marthinal CreditAttribution: marthinal commentedComment #3
jhodgdonThat definitely looks correct to me. Thanks for the patch!
Comment #4
jhodgdonAlthough actually we probably need a failing test for this bug before this patch will be accepted into Core.
Comment #5
marthinal CreditAttribution: marthinal commentedAdding the integration test to reproduce the bug.
@jhodgdon Thanks for reviewing!
Comment #6
marthinal CreditAttribution: marthinal commentedOops, sorry I forgot the test
Comment #8
jhodgdonLooks 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:
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...
Hm. This is a bit odd..
How about:
Tests searching with date filters that exclude some translations.
should be @var string[]
I think we also need 'locale' here?
login => log in
(when it's a verb, it's two words)
Where is the node content type itself set to be translatable? This seems to be missing something...
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.
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.
How about:
Tests searching with date filters that exclude some translations.
This isn't really an advanced search I think?
Do not use "nid" in comments. Say "node ID" instead.
spanish -> Spanish
Also ... We don't normally put @see in // comments. And it's lower-case if you did.
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.
This must have been copied from another module?
This @param is missing its description... but actually in hook implementations we normally don't include @param at all.
Comments should normally end in .
Comment #9
marthinal CreditAttribution: marthinal commented@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.
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!!!
Comment #10
jhodgdonThanks!
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:
Id -> ID
results does not => results do not
This file needs the @file doc block put back in.
Nice. Very clear one-line description here. :)
Comment #11
marthinal CreditAttribution: marthinal commentedOk! 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.
3) Done.
thanks!!!
Comment #12
jhodgdonOK, looks perfect now! Thanks!
Comment #13
marthinal CreditAttribution: marthinal commentedComment #14
alexpottCommitted e9d8f2d and pushed to 8.0.x, 8.1.x and 8.0.x. Thanks!
Fixed on commit - we still have not applied the new rule to core - so @file doc blocks are mandatory.