Add missing language functionality in search module

I will just note here that there were earlier (still open) issues regarding language and search in the Drupal Core issue queue (some even with patches). These are all concerns you will need to address if you want to make search work correctly with translated content:
#987210: Language-specific searches should include language neutral content
#511594: hook_search_preprocess needs to be language-specific

+++ b/core/modules/node/node.api.phpundefined
@@ -670,7 +672,7 @@ function hook_node_prepare(Drupal\node\Node $node) {
+function hook_node_search_result(Drupal\node\Node $node, $langcode) {

does it have to be there? Can't we assume it can be 'UND' or NULL also?

+++ b/core/modules/node/node.moduleundefined
@@ -2630,21 +2631,23 @@ function _node_index_node(Node $node) {
+  foreach ($node->translations() as $language) {
+    // Render the node in this language.
+    $build = node_view($node, 'search_index', $language->langcode);
+    unset($build['#theme']);

this is not very scalable. Suppose you index 50 items at once, you have 5 languages, so it will index 250 items at once.

Would it be better to have a separate indexing process per language of have the increment of the limit effect this foreach also?

+++ b/core/modules/search/search.api.phpundefined
@@ -224,28 +224,30 @@ function hook_search_execute($keys = NULL, $conditions = NULL) {
-    node_build_content($node, 'search_result');
-    $node->body = drupal_render($node->content);
+    $build = node_view($node, 'search_result', $item->langcode);

are we sure there is no performance impact here? I haven't delved into the difference between the two. Looks ok though, since we use the node_view in earlier execution.

I went ahead, and fixed a few things, so the tests should pass now -- at least I hope. :)

I don't know what you think about it, but I added a new method: _node_index_node_language_variant().
This indexes a language variant of a node, and it's called by _node_index_node(). Since Entity::translations() doesn't return the language which is set in $node->langcode, we need to render the node for the indexing once with this langcode, and then with using the other languages, that's why I thought it's a good idea to have a separated method that indexes a single language variant. What do you think?

I also implemented a simple counter that checks when is the maximum number of indexable items being reached, so the indexing throttle can be aware of the language variants. My idea was that a node should be indexed only if all its language variants can be indexed without exceeding the specified limit. Do you agree with this approach? I added a new test class: SearchMultilingualEntityTest, so far it attempts to test this indexing throttle functionality.

Fingers crossed for the tests. :)

+++ b/core/modules/node/node.moduleundefined
@@ -2623,7 +2624,13 @@ function node_update_index() {
+    $counter += 1 + count($node->translations());

I like this, but perhaps this can be documented a little bit more?
I'm not sure if people that read the source code will understand what this does.

Not a blocker though!

+++ b/core/modules/node/node.moduleundefined
@@ -2641,23 +2647,36 @@ function _node_index_node(Node $node) {
+    _node_index_node_language_variant($node, $language->langcode);

This will be confusing, don't you think?

Within _node_index_node there is a function that is also capable of indexing content called _node_index_node_language_variant? That's confusing :-)
I'm in favor of keeping this in 1 function. You could use a helper function perhaps but at least leave the search_index() calls in the _node_index_node()

Aside of that, very good work with the tests and implementing the throttle capabilities!

This should just be a working reroll of #10 for the latest 8.x

merge the node variant indexer as per Nick_vh (I'm fine either way :)

-- Do we need to invoke search_index() both after the original node is built and during the loop of its translations? Would _node_index_node_language_variant need to return the built variations as $node?

Marking needs review to ensure the reroll was successful - passed tests locally.

Hoping this passes.

Includes reroll from #15 with correct implementation of for comment_node_update_index() failures.

-    $node->rendered .= ' ' . module_invoke('comment', 'node_update_index', $node);
+    $node->rendered .= ' ' . module_invoke('comment', 'node_update_index', $node->langcode);
 
-    $extra = module_invoke_all('node_search_result', $node);
+    $extra = module_invoke_all('node_search_result', $node->langcode);

should have been:

-    $node->rendered .= ' ' . module_invoke('comment', 'node_update_index', $node);
+    $node->rendered .= ' ' . module_invoke('comment', 'node_update_index', $node, $node->langcode);
 
-    $extra = module_invoke_all('node_search_result', $node);
+    $extra = module_invoke_all('node_search_result', $node, $node->langcode);

I added a comment about the counter as: "Determine when the maximum number of indexable items is reached."
-- This may need to be revised to be more clear though...

I also refactored the helper function into _node_index_node() inside the loop.

Thanks for the reroll and refactoring. I noticed that the new test class (core/modules/search/lib/Drupal/search/Tests/SearchMultilingualEntityTest.php) had been removed in your patch, I guess that was an accident. :) So I added it back again (I didn't include it in the interdiff).

I have also changed the $node->language back to $item->language in node_search_execute() as Gábor pointed out that wasn't the correct thing to do in #10, since we need to render the node using the language in which the search has been executed. For some reason the language code doesn't appear in the $item, so I will continue and investigate this. But in the meantime I wanted to do this minor reroll and submit it to let us focus only on what's actually left.

I have added the language code to the result of the search query in node_search_execute(), so now it appears in $item. I also wrote a test for executing searches and checking the results as Gábor asked before. So it seems the todo list in #14 is completed.

Meanwhile some changes have been committed, so the patch doesn't apply anymore. I'll reroll it soon.

@balintk - I'd like to review the patch when it's ready so feel free to send me a direct message via my drupal.org contact page or through my blog contact form: http://www.kristen.org/contact so I can react quickly. I will be around all day today but not working tomorrow (Wed). Thanks!

Here is the rerolled patch.

Sorry, I didn't create an interdiff, because I removed the code hunks which caused the patch failing to apply, then manually added them again, so the interdiff would not have been correct.

Apparently we need the comment module to be enabled for some reason.

What I figured out: in node_search_execute(), the following line invokes comment_node_update_index():

+++ b/core/modules/node/node.moduleundefined
@@ -1619,28 +1619,32 @@ function node_search_execute($keys = NULL, $conditions = NULL) {
     // Fetch comments for snippet.
-    $node->rendered .= ' ' . module_invoke('comment', 'node_update_index', $node);
+    $node->rendered .= ' ' . module_invoke('comment', 'node_update_index', $node, $item->langcode);

Even though comment module is not enabled at this point. (So the function_exists() call returns TRUE in module_hook().) This doesn't happen when I run the tests locally.
The comment_node_update_index() method tries to fetch something from the database, but the comment table doesn't exist, that's why the fatal errors. Maybe this is a caching issue with the d.o. test environment. Honestly, I have no idea, but maybe it's trivial for some of you. :)

Anyway, the comment module is enabled now in the test, so it might solve the issue, but I'm not sure about that this is the right solution.

I looked through the code and found a few things but perhaps none of them require action:

+++ b/core/modules/search/lib/Drupal/search/Tests/SearchMatchTest.php
@@ -48,7 +48,7 @@ class SearchMatchTest extends SearchTestBase {
-      search_index($i, SEARCH_TYPE_JPN, $jpn);
+      search_index($i, SEARCH_TYPE_JPN, $jpn, LANGUAGE_NOT_SPECIFIED);

Just checking it is correct to use LANGUAGE_NOT_SPECIFIED when the search type is specifically for Japanese.

+++ b/core/modules/search/lib/Drupal/search/Tests/SearchMultilingualEntityTest.php
@@ -0,0 +1,105 @@
+    // Update the index and then run the shutdown method.
+    node_update_index();
+    search_update_totals();
+    $status = module_invoke('node', 'search_status');
+    $this->assertEqual($status['remaining'], 1, t('Remaining items after updating the search index is 1.'));

The comment says "run the shutdown method" yet I don't know what that means and don't see the word "shutdown" anywhere except in the comment. Is the comment understandable to the regular core contributors?

+++ b/core/modules/search/lib/Drupal/search/Tests/SearchMultilingualEntityTest.php
@@ -0,0 +1,105 @@
+    // Update the index and then run the shutdown method.
+    node_update_index();
+    search_update_totals();

Same here about "shutdown method."

+++ b/core/modules/search/search.api.php
@@ -224,28 +224,30 @@ function hook_search_execute($keys = NULL, $conditions = NULL) {
-    $node->rendered .= ' ' . module_invoke('comment', 'node_update_index', $node);
-    // Fetch terms for snippet.
-    $node->rendered .= ' ' . module_invoke('taxonomy', 'node_update_index', $node);
+    $node->rendered .= ' ' . module_invoke('comment', 'node_update_index', $node, $item->langcode);

Just checking that taxonomy is supposed to be removed from here and not updated like the comment code was... ?

Let me know if you want me to apply the patch to the latest D8 dev and, if so, how you'd like me to test. Thanks!

Thanks for the review!

Just checking it is correct to use LANGUAGE_NOT_SPECIFIED when the search type is specifically for Japanese.

In my opinion it's irrelevant what the language is here, so it's okay to use this constant.

The comment says "run the shutdown method" yet I don't know what that means and don't see the word "shutdown" anywhere except in the comment. Is the comment understandable to the regular core contributors?

Good point. The search_update_totals() method is normally called on cron shutdown. That's what the comment refers to. For instance in SearchAdvancedSearchFormTest.php the code is more commented:

<?php
// First update the index. This does the initial processing.
 node_update_index();

// Then, run the shutdown function. Testing is a unique case where indexing
// and searching has to happen in the same request, so running the shutdown
// function manually is needed to finish the indexing process.
search_update_totals();
?>

Do you think it would make more sense to copy over these lines?

Just checking that taxonomy is supposed to be removed from here and not updated like the comment code was... ?

That's the documentation that has been updated, and the example uses the node_search_execute() method as a sample hook_search_execute() implementation. Fetching the terms must be some legacy code there, but now it's updated according to the actual implementation.

The comments above regarding the "shutdown function" are clearer to me. Perhaps using those makes sense.

Thanks for clarification on the rest of it.

So... if I were to apply this patch, create some nodes with different languages, and test the search form, should it be obvious if it's working properly?

I added those comments, and also wrote some more. I hope it's clear enough now.

Since the tests cover the functionality I don't think you should do manual testing. Also, there is no UI in core for adding multilingual values to fields (yet).

I don't see a "needs backport" tag in this. That's because it breaks the API. Right? If so, could the changes here be somehow made available to D7 in the form of a contrib module or perhaps implemented in Backports? ...or is #1291388: Add support for multilingual core search supposed to tackle this?

Thanks for updating... one final comment (I hope!):

+++ b/core/modules/search/lib/Drupal/search/Tests/SearchMultilingualEntityTest.php
@@ -80,15 +80,23 @@ class SearchMultilingualEntityTest extends SearchTestBase {
   function testSearchingMultilingualFieldValues() {
     // Update the index and then run the shutdown method.
+    // (See the previous method for further explanation.)

My only suggestion would be to explicitly state which function has the extra comments in it, because additional functions might be added in the future within the file and then the "previous method" text might no longer be accurate. Not sure the parentheses are needed either... perhaps:

// See testingIndexThrottle() for further explanation.

Well, you are right. :) Here it is, with that better comment added.

Seems to progress really well!

I did not see any other todo's in the list and based on all the comments made earlier + the tests, I am going to mark this as RTBC. Change if you are opposed to this.

Thanks everyone! This is looking pretty good. I found a few things we need to fix:

1. +++ b/core/modules/node/node.moduleundefined
   @@ -1619,28 +1619,32 @@ function node_search_execute($keys = NULL, $conditions = NULL) {
   +    // Add the language code of the indexed item to the result of the query, since the node
   +    // will be rendered using the respective language.
   
   @@ -2653,7 +2657,14 @@ function node_update_index() {
   +  // The indexing throttle should be aware of the number of language variants of a node.
   
   +++ b/core/modules/search/lib/Drupal/search/Tests/SearchMultilingualEntityTest.phpundefined
   @@ -0,0 +1,113 @@
   +    // Make the body field translatable.
   +    // (The parent class has already created the article and page content types.)
   

   These comments (and many others in this patch) need to wrap at 80 characters. Reference: http://drupal.org/node/1354#general

2. +++ b/core/modules/search/lib/Drupal/search/Tests/SearchMultilingualEntityTest.phpundefined
   @@ -0,0 +1,113 @@
   +  protected $searchable_nodes = array();
   

   This member variable needs documentation. Reference: http://drupal.org/node/1354#classes

3. +++ b/core/modules/search/lib/Drupal/search/Tests/SearchMultilingualEntityTest.phpundefined
   @@ -0,0 +1,113 @@
   +  function setUp() {
   +    parent::setUp('language', 'locale', 'comment');
   

   This should be changed for #1380958: Replace $modules list for WebTestBase::setUp() with ::$modules class properties (see #1711070: Convert tests to use ::$modules property instead of parent::setUp($modules)).

4. +++ b/core/modules/search/lib/Drupal/search/Tests/SearchMultilingualEntityTest.phpundefined
   @@ -0,0 +1,113 @@
   +  function testIndexingThrottle() {
   ...
   +  function testSearchingMultilingualFieldValues() {
   

   Both these test methods need documentation. Reference: http://drupal.org/node/1354#functions

5. +++ b/core/modules/search/lib/Drupal/search/Tests/SearchMultilingualEntityTest.phpundefined
   @@ -0,0 +1,113 @@
   +    $this->assertEqual($status['remaining'], 1, t('Remaining items after updating the search index is 1.'));
   ...
   +      $this->assertEqual($search_result[0]['node']->nid, $node->nid, t('The search has resulted the correct node.'));
   

   The assertion messages should not use t(). Reference: http://drupal.org/simpletest-tutorial-drupal7#t

I've added this as a task for today's core mentoring. Thanks!

Fixed coding style issues.

Changed $modules to public.

I did a quick grep, and it looks like in most places in D8 core, $langcode defaults to NULL. Should it here as well? And if not, why not?

Settle down, I needed to get a couple of hours of sleep. ;)

I was more trying to suss out if there was a way to avoid developers having to deal with this new parameter in every case by defaulting it to a sensible value. I'm not sure that answers it, but I guess I'll go ahead and commit this and that can be a follow-up if necessary.

Committed and pushed to 8.x. Marking for change notification, since this requires developers implementing these hooks to do work.

Committed and pushed that CHANGELOG.txt entry to 8.x. Thanks! :)

I know this involves API changes and cannot be backported, but is there a solid solution for this in D7 (in contrib perhaps)? Everything I have tried in order to get language-aware search results so far failed.

Thanx Gábor!

On the face of it, throttling based on the number of translations seems to make sense, but it means the counters are inconsistent and introduces an edge case where indexing will be totally jammed.

I propose removing the language counter introduced in #10 and just throttling based on the number of nodes. If you have an opinion either way, please comment on #2178643: Indexing status can show a negative percentage of content as having been indexed