Multilingual node search bugs with title and language filtering

Very similar problem to #2006606: Views rendering ignores entity language, eg: Identical nodes rendered in views when nodes have translations and underlying label() API problem handled in #2142979: Entity label langcode argument is a lie, incompatible with current API. Needs tests.

1: searchfix-2142987-1.diff queued for re-testing.

1: searchfix-2142987-1.diff queued for re-testing.

1. +++ b/core/modules/node/lib/Drupal/node/Plugin/Search/NodeSearch.php
   @@ -326,6 +326,8 @@ protected function indexNode(EntityInterface $node) {
   +      // get the right translation with the new entity api.
   

   I don't think this comment is needed.

   1. Its not added in the execute either.
   2. get would be Get
   3. We don't say "new entity API" because by this is released, used, etc. it will be the old one :D

2. +++ b/core/modules/search/lib/Drupal/search/Tests/SearchMultilingualEntityTest.php
   @@ -52,20 +53,25 @@ function setUp() {
   +    // The title field is translatable by default
   +
   

   I would move this comment up to where the body is set to be translatable. I think that already has a comment for that, If not, do something like "Set body to be translatable. The title is already translatable by definition." Or something along those lines :)

3. +++ b/core/modules/search/lib/Drupal/search/Tests/SearchMultilingualEntityTest.php
   @@ -52,20 +53,25 @@ function setUp() {
   +        'title' => 'Second node this is the english title',
   
   @@ -76,14 +82,15 @@ function setUp() {
   +    $translation = $this->searchable_nodes[2]->addTranslation('hu', array('title' => 'Third node this is the hungarian title'));
   
   @@ -118,16 +125,20 @@ function testSearchingMultilingualFieldValues() {
   +    // this should find two results for the second and third node
   

   Comments should start with uppercase and let's use "English" and "Hungarian" properly cased :)

4. +++ b/core/modules/search/lib/Drupal/search/Tests/SearchMultilingualEntityTest.php
   @@ -118,16 +125,20 @@ function testSearchingMultilingualFieldValues() {
   +    // there's a bug: if both translations are on the same node, seach will find only one of them
   

   Remove this comment and open a new issue. We don't put comments about bugs in the code like this :D

5. +++ b/core/modules/search/lib/Drupal/search/Tests/SearchMultilingualEntityTest.php
   @@ -118,16 +125,20 @@ function testSearchingMultilingualFieldValues() {
   +    // another bug: this also returns both results.
   +    //$plugin->setSearch('english OR hungarian', array('f' => array('langcode:hu')), array());
   

   Yet another issue to open :D So my understanding is this finds the second node based on "english" in the title and the third node based on "hungarian" in the title but then renders both of them in the same language?! Looks like this would be another bug. Maybe in fact the same bug as the previous comment :)

6. +++ b/core/modules/search/lib/Drupal/search/Tests/SearchMultilingualEntityTest.php
   @@ -118,16 +125,20 @@ function testSearchingMultilingualFieldValues() {
   -}
   +}
   \ No newline at end of file
    
   

   Don't remove the newline :)

1. +++ b/core/modules/search/lib/Drupal/search/Tests/SearchMultilingualEntityTest.php
   @@ -40,13 +40,14 @@ function setUp() {
   +    // Make the body field translatable. The title is already translatable by definition.
   

   Wrap at 80 chars.

2. +++ b/core/modules/search/lib/Drupal/search/Tests/SearchMultilingualEntityTest.php
   @@ -56,16 +57,19 @@ function setUp() {
   +        'title' => 'Second node this is the english title',
   
   @@ -76,14 +80,15 @@ function setUp() {
   +    $translation = $this->searchable_nodes[2]->addTranslation('hu', array('title' => 'Third node this is the hungarian title'));
   
   @@ -118,16 +123,20 @@ function testSearchingMultilingualFieldValues() {
   +    $plugin->setSearch('english OR hungarian', array(), array());
   ...
   +    $this->assertEqual($search_result[0]['title'], 'Third node this is the hungarian title', 'The search finds the correct hungarian title.');
   +    $this->assertEqual($search_result[1]['title'], 'Second node this is the english title', 'The search finds the correct english title.');
   ...
   +    $plugin->setSearch('english OR hungarian', array('f' => array('langcode:hu')), array());
   ...
   +    $this->assertEqual($search_result[0]['title'], 'Third node this is the hungarian title', 'The search finds the correct hungarian title.');
   

   english => English
   hungarian => Hungarian

3. +++ b/core/modules/search/lib/Drupal/search/Tests/SearchMultilingualEntityTest.php
   @@ -118,16 +123,20 @@ function testSearchingMultilingualFieldValues() {
   +    //This should find two results for the second and third node
   ...
   +    // Now filter for hungarian results only
   

   // followed by a space
   dot at the end.

   (In short always a full sentence).

Updated issue summary extensively explaining all bugs. Retitled to cover the whole scope.

Further clarification of problem (3).

16: searchfix-with-tests-4.diff queued for re-testing.

Good job k4v, this fixes all the right things the right ways and updates tests to be meaningfully looking for translations as well as prove the language filter works (unlike before :).

This is a rather odd and non-intuitive DX. It looks like if I load a node and want to render it in a particular translation, I not only have to pass the language into the ->view() method, but before that I have to call getTranslation()? Why is that?

-      $node = $node_storage->load($item->sid);
+      $node = $node_storage->load($item->sid)->getTranslation($item->langcode);
       $build = $node_render->view($node, 'search_result', $item->langcode);

Anyway... That aside, this looks like a good fix to me and the tests are clear. I like that. :)

@jhodgdon: yeah the reason we need to do this is as follows:

1. We need getTranslation() because we need the title in the right language, and the current entity API works so you get whole variants of the entity and don't get properties in that language one by one.

2. However, if we don't pass on a langcode to the view method, it ends up in https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Entity%21... which then takes the negotiated language for the page and will get that translation.

So if we want to have an explicit language, we need to pass it on although we already loaded that translation. That is the current API. I submitted #2144251: EntityViewBuilder may display different language if langcode is not specified after discussion with @fago and looking at https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Entity%21...

tl;dr: the patch uses the current API as good as possible :)

Thanks for the explanation!

Elevating due to the amount of bugs this ended up covering.

Just minor code style nitpicks, otherwise looks good:

1. +++ b/core/modules/search/lib/Drupal/search/Tests/SearchMultilingualEntityTest.php
   @@ -40,14 +40,17 @@ function setUp() {
        $field = field_info_field('node', 'body');
   

   Extraneous blank line here.

2. +++ b/core/modules/search/lib/Drupal/search/Tests/SearchMultilingualEntityTest.php
   @@ -118,16 +125,19 @@ function testSearchingMultilingualFieldValues() {
   +    //This should find two results for the second and third node.
   

   Needs a space after //.

3. +++ b/core/modules/search/lib/Drupal/search/Tests/SearchMultilingualEntityTest.php
   @@ -118,16 +125,19 @@ function testSearchingMultilingualFieldValues() {
   +    // Now filter for hungarian results only.
   

   Hungarian should be capitalised, see also another hungarian and English below.

With those concerns fixed, and tests passing again back to RTBC.

Committed/pushed to 8.x, thanks!

Yay, superb, thanks!