The purpose of http://api.drupal.org/api/function/hook_search_preprocess/7 is to allow stemming (and perhaps other) modules to pre-process text before it is added to the index in the search module, or used as input to a query on the search index. As it is currently in Drupal 6 and 7, the hook has only a single parameter, which is the text to be pre-processed.

That works fine for a single-language site, where you would likely only have a single stemming module. But for a multi-lingual site, with presumably multiple stemming modules, the stemming modules have no way of knowing which language the text is in. And you definitely shouldn't pre-process English text with a German or Spanish stemming module, or vice versa. See this issue on the Porter Stemmer project, for instance: #363336: Add option for Porter-stemmer to only stem English or language neutral content for a multi-language site.

Given that, I think that hook_search_preprocess needs to have a second input giving the language the text is in, so that stemming modules can decide not to change the text if it is not in their language. For that to happen, several other functions would also need to have new language arguments. Let's see...
- hook_search_preprocess() is invoked from http://api.drupal.org/api/function/search_invoke_preprocess/7
- search_invoke_preprocess() is called from http://api.drupal.org/api/function/search_simplify/7 (actually, that search_invoke_preprocesss function seems kind of pointless, since it is 3 lines and only called in one place?)
- search_simplify() is called from both http://api.drupal.org/api/function/search_index_split/7 and http://api.drupal.org/api/function/search_parse_query/7
- search_index_split() is called from http://api.drupal.org/api/function/search_index/7 -- that is the search API function that modules can call to cause text to be indexed during their hook_search_index() implementations.
- search_parse_query() is called from http://api.drupal.org/api/function/do_search/7 -- that is the search API function that modules can call to do a search query.

So all the functions listed above would need to have language awareness. At the API level, search_index() would need to have a language input parameter that the modules would pass in to tell search_index() what language the text is in. And do_search() should be able to glean from the environment what language is in use by the user doing the search (or have that be an input to the function with a default being the current language).

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhodgdon’s picture

Status: Active » Needs review
FileSize
9.44 KB

Here's a patch. Definitely needs review, as it involves some API changes.

jhodgdon’s picture

It would be great to get this into D7 before the code freeze. The next step would probably be if someone could take the time to review the patch. It really is essential for a multi-lingual site that wants to use the core Search module and wants it to have stemming so people can actually find things on the site without typing the exact form of the word.

jhodgdon’s picture

Issue tags: +API change

Status: Needs review » Needs work

The last submitted patch failed testing.

douggreen’s picture

Priority: Normal » Critical

This needs to be rewritten for DBTNG. I'm marking as critical, multilingual support for search is long overdue... and would be embarrassing not to have in 7.x. See also #335928: Thai vowels are excluded in search index.

jhodgdon’s picture

I was looking into rewriting this patch.... It appears that the entire search API has been totally rewritten. But there is nothing on http://drupal.org/update/modules/6/7 to explain it or even give a link to the issue. Can someone enlighten me?

Or better yet, put it on the module update guide?

douggreen’s picture

Are you referring to #394182: DBTNG search.module? I don't think that this was an entire rewrite of the search API, but rather a re-organization to accommodate the new database changes in D7.

jhodgdon’s picture

That's at least some of it... The patch that was committed (http://drupal.org/files/issues/search_dbtng_7.patch) removed functions do_search(), search_parse_query(), which is definitely a change in the search API, which wasn't mentioned in the upgrade guide.

jhodgdon’s picture

So I guess my question becomes: where do I need to add the changes of this patch?

Language needs to be passed around to stemming/preprocessing modules during indexing and during searching.

I think the indexing code is fairly similar to what it was, so probably can do the same thing...

But in my patch above, for the searching part, I added code to search_parse_query(), to call the language-aware search_simplify(). But that function no longer exists. And I don't see similar code anywhere -- the only place that is calling search_simplify() in D7 is during indexing -- it's not being called during searching (possibly this is a bug? I cannot be sure.).

Any clues?

jhodgdon’s picture

Aha, I found it, in search.extender.inc

api.drupal.org of course doesn't index classes. Sigh. That really needs to be fixed before Drupal 7 is released into the wild, because the doc indexing of functions calling functions and so forth is rather worthless without the many classes being included.

Double sigh.

Anyway, I think I can now figure out how to patch this again...

douggreen’s picture

Good point on the API docs. Can you bring this up with webchick or the docs team? Or should I?

jhodgdon’s picture

There's already a critical issue on the API project:
#300031: Rework PHP parser

Doc team is not the right place to bring this up, as api.drupal.org is under Drupal project not Docs.

I'll send a message to webchick.

jhodgdon’s picture

Assigned: Unassigned » jhodgdon

Actually I am just going to file an issue against D7.
#594192: Do not release Drupal 7 until api.drupal.org supports classes

Anyway, I'll rework this patch, but not until next week, as I'm teaching a beginning Drupal workshop today.

jhodgdon’s picture

Status: Needs work » Needs review
FileSize
16.2 KB

Here's a rework of the patch.

jhodgdon’s picture

Is this something that needs to get in today or be relegated to Drupal 8? If so, douggreen (who marked this critical) - do you have any interest in reviewing it?

Status: Needs review » Needs work

The last submitted patch failed testing.

jhodgdon’s picture

Should I bother to try to re-roll this patch? It would be an API change, so I am guessing the code freeze means that it would not go in until Drupal 8?
http://drupal.org/node/619258

boran’s picture

I consider this fix important: D7 needs to be multi-lang and fix many of the outstanding issues with D6, of which multilang search/stemming is an important one.
So if the core developers would reconsider pulling this into D7, it would be appreciated.

jhodgdon’s picture

Status: Needs work » Needs review
FileSize
16.23 KB

Just in case this has a chance of getting into D7, here's an updated patch.

Status: Needs review » Needs work

The last submitted patch failed testing.

jhodgdon’s picture

That is odd. I'm sure not seeing a PHP syntax error with that patch applied... ???

Status: Needs work » Needs review

jhodgdon requested that failed test be re-tested.

Dries’s picture

Status: Needs review » Needs work
+++ modules/search/search.pages.inc	20 Nov 2009 15:52:35 -0000
@@ -10,6 +10,8 @@
 function search_view($type = 'node') {
+  global $language;

Just using the global $language might not be desired -- and changes the current behavior.

+++ modules/search/search.pages.inc	20 Nov 2009 15:52:35 -0000
@@ -29,7 +31,7 @@
+      $results = search_data($keys, $type, $language->language);

At first glance, it looks like we need to give this more thought. For example, in Belgium there are 3 official languages -- how can I specify multiple languages? The new API seems to assume that people care about one language only.

Specifying one language makes sense for the indexing functions, but not for the search functionality itself.

+++ modules/search/search.module	20 Nov 2009 15:52:35 -0000
@@ -352,8 +352,15 @@
+ * @param $lang

We typically don't abbreviate words but in this case it might be OK.

+++ modules/node/node.module	20 Nov 2009 15:52:35 -0000
@@ -1528,14 +1528,28 @@
+function node_search_execute($keys = NULL, $lang = NULL) {

What if I want to search multiple languages (but not all languages)?

+++ modules/node/node.module	20 Nov 2009 15:52:35 -0000
@@ -1528,14 +1528,28 @@
+  // Override the language passed in if the user specified language.
+  $tmp_lang = search_expression_extract($keys, 'language');
+  if ($tmp_lang) {
+    if (strpos( $tmp_lang, ',') !== FALSE) {
+      // multiple selection
+      $lang = NULL;
+    }
+    else {
+      // single selection
+      $lang = $tmp_lang;
+    }
+  }

Various code style issues: comments not in right format, etc.

jhodgdon’s picture

Thanks very much for the review. Style issues noted, will fix in next patch (if I make one).

So as regards to functionality... This patch definitely would change the current behavior for search. Which is why I posted this issue in July, and made an effort (on IRC, posting in the Search group on g.d.o, I think I posted on the dev mailing list as well) for this to get reviewed before the API freeze (though as you can see, no one paid any attention). And why I think probably this change now needs to be pushed back to Drupal 8 for more discussion, if in fact anyone will bother to discuss it even then. [And yes, for those of us who are not recognized members of the "core" Drupal team, the process is broken. Which is why I have been since this experience only doing doc patches, because they are pretty much non-controversial and can actually get committed.]

Anyway, just to explain the rational of this patch... You do have to make the search step, as well as the indexing step, somehow be language-aware for this to work.

The reason is that if I index in a language-aware way... Let's take an example. Say I have an English-Spanish site and I have installed both English and Spanish stemming modules. When I index the text on the pages, the English text is only English stemmed, and Spanish text only Spanish stemmed (with this patch, anyway).

Then I go to search. Say I enter an English word as my search term. In order for it to match a word on one of the English pages, it has to be run just through the English stemmer, not through both (because the Spanish stemmer could change my English word -- these stemmers work on patterns/algorithms, not with a word list).

So Search either needs to:
- Make a guess as to the language of the words you entered (and global $language would be the guess, since that is the UI language you are currently using).
- Have radio buttons on the search form so you can choose the language (in which case, probably you'd want to also add to the query so only text in that language was returned).
- Run the search terms through all languages separately -- e.g. [conceptually] search for a match of any of the following: preprocess($term, 'en'), preprocess($term,'es'), preprocess($term,NULL).\

What you don't want is to preprocess text with language specified, then essentially run search terms through preprocess(preprocess($text,'en'),'es').

jhodgdon’s picture

Version: 7.x-dev » 8.x-dev

It looks like this needs to (unfortunately, due to lack of interest earlier on for reviewing this) be pushed to Drupal 8. Too bad!

jhodgdon’s picture

Category: bug » feature
Priority: Critical » Normal

If it's pushed to D8 it needs to be a feature request.

Just a thought: We probably need to pass the language object and not language code around...

klonos’s picture

Coming from #278443: Make hook_search_preprocess language aware in order to support multi-language stemmers...

Let me just note that this approach here of having the case of NULL if unknown/multiple is bad practice in reality. These are two entirely different cases and should not be treated as the same. I do realize that "stemmers work on patterns/algorithms, not with a word list", but making the assumption that the text being indexed or searched is single-language and treating (actually ignoring) all other cases as if they are "language neutral" will simply bring bad results (plus it beats half of the issue's purpose IMHO).

Most software out there start in English and multilingualism is not thought of until -almost- too late, after the foundations are already set. Drupal is no different when it comes to that, despite the hard efforts being worked on in various projects. I am not saying it will be easy (code-wise I mean), but we cannot simply continue the bad practice of ignoring half of the issue in order to move things on. Drupal 8 will have to be truly multilanguage - in every aspect.

What is being discussed here is good and all, but making search language-aware is half the job done. We need to figure out what to do in cases where both the text in content and the text in search queries is consisted of a multitude of languages. Most common use case scenario is IMO when a text is in a certain non-English language in its whole, but some words such as names etc are kept in their English form. Nobody for example would try to translate 'Drupal' or 'Greenpeace'. At the very least we should figure out a way to have this fact under consideration.

PS: Thanx for all your hard work on this one Jennifer! I too hate it that this did not make it in D7. It would be a nice start at the very least.

Simon Georges’s picture

As part of the team that maintains French Stemmer module, I'm subscribing to that !

klonos’s picture

- Consider a site with English and Greek enabled.
- Let's say that all content is either one or the other language -neutral not allowed-, because I honestly cannot think of any such use case (English-Greek, English-Arabic, English-Chinese etc) that would allow a node to be language-neutral.
- Let's also say that a site author adds an article about Drupal in English and then leaves the translation of this article in Greek to be done at some later point by the site's Geek translator.

What happens the way it is now...

In the mean time, a Greek visitor -that also speaks English- comes to the site and searches the term "Drupal" while switched to the Greek UI. They will see no search results! Not until they either switch the site's UI to English or they wait for the Greek translator to finish with the article's translation.

What should happen is...

1. If the search term is not found in the user's language but there are results in other languages, then this fact should be known to the user. For example, instead of only trowing a "No results found" at them, we could be allowing them to view those other-language results: "No results found in your current language. See results in other languages...". Or simply show the other language results anyways and let the user know: "No results found in your current language. Here is a list of results in other languages...".

2. If the search term is found in the user's language and other languages as well, then "promote" the results that are in the user's language and list the other languages last.

3. No results found whatsoever. Oh well! what can one do? ;)

PS: both cases 1 & 2 could use a solr-search type widget there to allow users to filter languages in case there are too many enabled in the site.

jhodgdon’s picture

If you want complex behavior like that, you might consider using Views instead of the core search.

klonos’s picture

I am no views-ninja, but you are right that I most likely could achieve such behavior using views. One thing though... it might be a complex behavior, but the use case is not complex at all. In fact I'd say it's very common (most if not almost all sites with English/non-Latin pairs of languages). So a solution that works out of the box is highly preferred - we're not talking about just me here.

klonos’s picture

...unless of course when you said "complex" you were referring to the language filter I propose in my PS in #29. That was just a "bonus" request not a requirement ;)

Anyways, it still remains a fact that if this is not resolved, there are cases where content is not returned as a result while it should.

jhodgdon’s picture

Assigned: jhodgdon » Unassigned

Not working on search.module any more, sorry.

klonos’s picture

Issue tags: +D8MI
Gábor Hojtsy’s picture

Now that #1669876: Add missing language functionality in search module landed, some of this is outdated.

sxnc’s picture

Status: Needs work » Needs review
FileSize
3.78 KB

Olright, i managed to create a patch that should add the $langauge object to the hook_search_preprocess (atleast it works on my local)

Nick_vh’s picture

Status: Needs review » Needs work

We did a manual code review and he needs to work a bit more on the default value when the parameter is not defined. I suggested to take a look at porterstemmer for drupal 7

sxnc’s picture

Status: Needs work » Needs review
FileSize
4.19 KB

Okay! I checked it again and with a little help from vasi1186 i left the $language default to NULL. In this case the undefinded language object will be passed along.

sxnc’s picture

dang it, forgot to update search.api.php, here's an updated one~

Status: Needs review » Needs work

The last submitted patch, adding_language_to_hook_511594-39.patch, failed testing.

sxnc’s picture

Alright, changed patch so that all functions have a default value and instead of the language object i now pass along the langcode only.

sxnc’s picture

Status: Needs work » Needs review

status update

sxnc’s picture

FileSize
642 bytes

here's a small module that invokes the hook so you can test it easier (i guess this is the easiest way to test this?).

YesCT’s picture

+++ b/core/modules/search/search.api.php
@@ -312,7 +312,12 @@ function hook_search_page($results) {
+function hook_search_preprocess($text, $langcode = NULL) {
+  // If language is not specified we pass along the
+  // unspecified langcode.
+  if (!$langcode) {
+    $langcode = LANGUAGE_NOT_SPECIFIED;
+  }

I was wondering why making the default NULL and then right away assuming LANGUAGE_NOT_SPECIFIED. I thought maybe just make the default LANGUAGE_NOT_SPECIFIED. I asked sxnc and they say it's because in the ui, it's possible not to check any check boxes in the search, advanced search field set and this passes NULL. This "means" to me the same thing. Because of being able to not check any boxes, I think should keep NULL there.

Powered by Dreditor.

YesCT’s picture

Gabor explained to me that we should keep NULL as the default in the parameter list... AND we should not then change to LANGUAGE_NOT_SPECIFIED so that contrib can have all the information.

Gábor Hojtsy’s picture

Yes, the simplifier is used in both the indexing and the search keyword processing. In the search keyword processing, passing on that "we don't know anything about the language" is good, so NULL is the best we can tell. The lower layer can still do assumptions like take the interface language, so this way we give the most data and provide the most flexibility. Also, the patch becomes simpler :)

We should document this on the phpdoc of hook_search_preprocess(), so people are aware. I think something like:

"If we don't know about the language (eg. search form was used), the language code will be NULL. In indexing, the language code is passed on from the entity being indexed".

Helpful? :)

sxnc’s picture

olright, after some discussions with YesCT and Gabor i removed the fallback to LANGUAGE_NOT_SPECIFIED so that NULL will remain when its passed along.

Nick_vh’s picture

Status: Needs review » Needs work
+++ b/core/modules/search/search.moduleundefined
@@ -554,9 +554,9 @@ function _search_index_truncate(&$text) {
+    $text = module_invoke($module, 'search_preprocess', $text, $langcode);
+++ b/core/modules/search/search.api.phpundefined
@@ -312,7 +312,7 @@ function hook_search_page($results) {
   return $text;

Also the API file needs a little example on how to use the langcode

So the only issue I see is this API change, you've done a great job sxnc in making this happen. Do you know the procedure of the change in the API or do you need some help there?

YesCT’s picture

FileSize
558 bytes

Tested functionality of patch in #47
Found some other search issues I will try to find issues for/create issues.
But, the result of looking at the language part of searching, from the changes in this patch is that:
*it worked*

Here is the detail for how sxnc and I did the testing:

Getting ready to test

drush dl devel
get patch
get module
drush -y en devel custom

with no content

search returns no result and null in the dpm
good

with content

http://localhost:8888/d8-git/node/1#overlay=admin/config/search/settings
reindex site: drush search-index
check was actually indexed
http://localhost:8888/d8-git/search/node/hello#overlay=admin/config/sear...
yes

search for something

dpm for $text and $language will always show NULL for language because we do not know the language of the search terms, the checkboxes in the advanced search field set are specifying the language of the content we want to look in.

to see the dpm with some value for $language, we need to watchdog and see it ... ( during the indexing? ) during the search?

look in
http://localhost:8888/d8-git/search/node/hello#overlay=admin/reports/dblog

was getting confused by seeming caching of previous search but was result of something with overlay
http://localhost:8888/d8-git/search/node/blah#overlay=admin/reports/dblog
so open watchdog log messages with
http://localhost:8888/d8-git/admin/reports/dblog

NOTE for testing do not use search block

clear log messages
no content
no search terms
drush search-index
verify search_dataset database table is empty
reload recent log messages
no messages
ok

search for something not found (fff)

but no content so anything searched for will not be found

reload recent log messages
3 messages
searched for fff
ok
text fff
language code NULL
ok because search terms will always have NULL

Get ready to find something, so make content

make content (just fill out title and body)
drush search-index
two invocations, one for title, one for body
lang code NULL for both
ok

search for something to found in content

search term has NULL
the content strings have en
ok

More complicated testing with languages

enable modules:
content translation (translation)
language
locale
drush -y en translation language locale
(did not work, enable in ui)

to make some language choices

in order to make choices
edit content type and uncheck hide language selector
make content
set language to not specifice
drush search-index
clear log
search for something to be found
the content strings have und
ok
search term has NULL
ok

repeated for mul (language: multiple language)

same (with mul)
ok

add a laungage

http://localhost:8888/d8-git/node/3#overlay-context=search/node&overlay=...
make some content
drush search-index
clear log
ok

Overall Result

it works

Nick_vh’s picture

So we should add some simpletests to make all these test cases in simpletest and reproducable test cases right? We can use that module of yours to get a good start!

YesCT’s picture

sxnc’s picture

Status: Needs work » Needs review
FileSize
4.47 KB

Olright++
So i updated the patch now with a short description in the API here:

* @param $text
 *   The text to preprocess. This is a single piece of plain text extracted
 *   from between two HTML tags or from the search query. It will not contain
 *   any HTML entities or HTML tags.
 *
 * @param $langcode
 *   The language code of the entitiy that has been found.
 *
 * @return
 *   The text after preprocessing. Note that if your module decides not to alter
 *   the text, it should return the original text. Also, after preprocessing,
 *   words in the text should be separated by a space.
 *

Not sure what i should write for this:

+++ b/core/modules/search/search.moduleundefined
@@ -554,9 +554,9 @@ function _search_index_truncate(&$text) {
+    $text = module_invoke($module, 'search_preprocess', $text, $langcode);

any ideas?

Then with the help of vasi1186 i made a new simpletest that checks for the language which is also included in the patch.

ps: @Nick_vh

So the only issue I see is this API change, you've done a great job sxnc in making this happen. Do you know the procedure of the change in the API or do you need some help there?

Thanks, i try to help wherever i can :D i asked Gabor about it and he said the following:
AFTER the patch is committed, you go to http://drupal.org/node/add/changenotice and create a change notice node. You fill in the description of the change "Language information passed to search hooks" or something (8.x as branch, 8.0 as version).

Then the issue title is a node reference and you write a summary of the changes with before/after code examples -> http://drupal.org/list-changes/drupal

So the node will appear here and people will be able to see it when updating their modules and it is cross-linked automatically under the issue summary on the issue.

e.g., this is one of ours: http://drupal.org/node/1749954

Nick_vh’s picture

I don't see the simpletest?

Nick_vh’s picture

Status: Needs review » Needs work
sxnc’s picture

Status: Needs work » Needs review
FileSize
7.57 KB

@nick_vh

Oh, sorry didn't add them to the patch correctly :( But now it should be right!

attiks’s picture

Status: Needs review » Needs work

quick code review:

+++ b/core/modules/search/tests/modules/search_langcode_test/search_langcode_test.moduleundefined
@@ -0,0 +1,5 @@
+<?php
+	function search_langcode_test_search_preprocess($text, $language) {
+		drupal_set_message('Langcode Preprocess Test: ' . $language);
+		return $text;

spacing problem

sxnc’s picture

Status: Needs work » Needs review
FileSize
7.57 KB

@attiks

Ugh, my SublimeText 2 keeps setting the tab space to 4... didnt notice updated the patch again~

sxnc’s picture

ignore the last patch, it looked like 2 spaces in sublime what was in fact still 4 spaces wide, added new patch with correct spacing.

attiks’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me

Nick_vh’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/search/search.api.phpundefined
@@ -312,7 +315,7 @@ function hook_search_page($results) {
   return $text;

I'd still add an example here that uses the langcode

+++ b/core/modules/search/tests/modules/search_langcode_test/search_langcode_test.moduleundefined
@@ -0,0 +1,5 @@
+    drupal_set_message('Langcode Preprocess Test: ' . $language);

can we add a 'stemming' test? For example, you add text : 'we are testing'

the preprocess should add
1) or all the verb forms of test/tested/testing
2) shape back the 'testing' to test, and you should test it by executing a search query for test.

This preprocess should only happen if the language is en, so the test with an undefined language should not be able to find 'test' in contrary to 'testing'

is that clear enough?

4 days to next Drupal core point release.

sxnc’s picture

Status: Needs work » Needs review
FileSize
10.27 KB

Added a small example for:

+++ b/core/modules/search/search.api.phpundefined
@@ -312,7 +315,7 @@ function hook_search_page($results) {
   return $text;

Also extended the test with a small stemming-like test

Nick_vh’s picture

Status: Needs review » Needs work

Great work so far! Just a few small comments and I think we are almost ready to mark it RTBC

+++ b/core/modules/search/search.api.phpundefined
@@ -312,8 +315,16 @@ function hook_search_page($results) {
+  // $langcode example: ¶

trailing whitespace

+++ b/core/modules/search/tests/modules/search_langcode_test/search_langcode_test.moduleundefined
@@ -0,0 +1,16 @@
+  function search_langcode_test_search_preprocess($text, $langcode) {

Function does not have documentation yet

+++ b/core/modules/search/tests/modules/search_langcode_test/search_langcode_test.moduleundefined
@@ -0,0 +1,16 @@
+      } else {

I *think* the code style implies the else should be on the next line

+++ b/core/modules/search/tests/modules/search_langcode_test/search_langcode_test.moduleundefined
@@ -0,0 +1,16 @@
+    } elseif (isset($langcode)) {

same for elseif, should be on the next line. But please correct me if I'm wrong

3 days to next Drupal core point release.

sxnc’s picture

Status: Needs work » Needs review
FileSize
10.34 KB

trailing whitespace

removed!

Function does not have documentation yet

added!

I *think* the code style implies the else should be on the next line & same for elseif, should be on the next line. But please correct me if I'm wrong

You were right, it needs to be on a new line :)
http://drupal.org/coding-standards#operators

attached updated patch~

sxnc’s picture

I managed to get my Code Sniffer to run again and fixed a few issues coding standards.

Nick_vh’s picture

Status: Needs review » Needs work
+++ b/core/modules/search/lib/Drupal/search/Tests/SearchPreprocessLangcodeTest.phpundefined
@@ -0,0 +1,100 @@
+  public static function getInfo() {

nitpicking here, does our code standards imply we need a description of every function? or does getInfo not need some comments anymore. Imho not enough weight to mark this patch as needs work again.

+++ b/core/modules/search/tests/modules/search_langcode_test/search_langcode_test.moduleundefined
@@ -0,0 +1,27 @@
+function search_langcode_test_search_preprocess($text, $langcode) {

nitpicking again, but if you check if $langcode is not set, you should set the $langcode = NULL in the function definition.

Again, very minor!

I'll tell you! We are very very close!

3 days to next Drupal core point release.

sxnc’s picture

Status: Needs work » Needs review
FileSize
10.5 KB

nitpicking here, does our code standards imply we need a description of every function? or does getInfo not need some comments anymore. Imho not enough weight to mark this patch as needs work again.

I saw this too when i ran Code Sniffer but after checking some of the other tests inside search i couldn't find any that had a description for it either. I left it as it is for now though~

nitpicking again, but if you check if $langcode is not set, you should set the $langcode = NULL in the function definition.

added!

Nick_vh’s picture

Status: Needs review » Reviewed & tested by the community

Good to go!

YesCT’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/search/lib/Drupal/search/Tests/SearchPreprocessLangcodeTest.phpundefined
@@ -0,0 +1,100 @@
+    // Checks if the langcode has been passed by the Preprocess Hook.

use the hook function name instead. Like:

// Checks if the langcode has been passed by hook_search_preprocess().

+++ b/core/modules/search/lib/Drupal/search/Tests/SearchPreprocessLangcodeTest.phpundefined
@@ -0,0 +1,100 @@
+   * Small stemming test for hook_search_preprocess().

Maybe this should be of the form that starts with Tests like:

Tests stemming for hook_search_preprocess().

+++ b/core/modules/search/search.api.phpundefined
@@ -312,8 +315,16 @@ function hook_search_page($results) {
+  // If the langcode is set and is 'en' then add 'en_text' to each
+  // $text. Using this everything with $langcode 'en' will also
+  // be found by 'en_text'.

I think this might fit on two lines... or at least stretch closer to the 80 char limit.

+++ b/core/modules/search/tests/modules/search_langcode_test/search_langcode_test.moduleundefined
@@ -0,0 +1,27 @@
+ * Test module implementing a 2 tests, one for the supplied entity $langcode
+ * and a small stemming test.

just take out the "a" so it is like:

* Test module implementing two tests, one for the supplied entity $langcode
* and another small stemming test.

+++ b/core/modules/search/tests/modules/search_langcode_test/search_langcode_test.moduleundefined
@@ -0,0 +1,27 @@
+    // Small stemming test.

Not a sentence.

Use instead something like:

// This small stemming test tests ... whatever it actually does.

sxnc’s picture

Status: Needs work » Needs review
FileSize
10.62 KB

Fixed the comment issues from #68

Status: Needs review » Needs work
Issue tags: -multilingual, -API change, -D8MI

The last submitted patch, adding_language_to_hook_511594-69.patch, failed testing.

Nick_vh’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +multilingual, +API change, +D8MI

The last submitted patch, adding_language_to_hook_511594-69.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
858 bytes
10.62 KB

Looks like a closing curly bracket got removed in the comment fix. Simple fix.

Nick_vh’s picture

Status: Needs review » Reviewed & tested by the community
webchick’s picture

Status: Reviewed & tested by the community » Fixed
+++ b/core/modules/search/search.api.phpundefined
@@ -312,8 +315,15 @@ function hook_search_page($results) {
+  // $langcode example:
+  // If the langcode is set and is 'en' then add 'en_text' to each $text. Using
+  // this everything with $langcode 'en' will also be found by 'en_text'.
+  if (isset($langcode) && $langcode == 'en') {
+    $text .= 'en_text';
+  }

+++ b/core/modules/search/tests/modules/search_langcode_test/search_langcode_test.moduleundefined
@@ -0,0 +1,28 @@
+  if (isset($langcode) && $langcode == 'en') {
+    // Add the alternate verb forms for the word "testing".
+    if ($text == 'we are testing') {
+      $text .= ' test tested';
+    }

I moved the current test stuff into this example, because what was there before doesn't make a lot of sense as an example.

Elsewise, this looks good! Committed and pushed to 8.x. Thanks!

Nick_vh’s picture

woohoo! Thanks to all that helped :)

Gábor Hojtsy’s picture

Title: hook_search_preprocess needs to be language-specific » Change notice: hook_search_preprocess needs to be language-specific
Category: feature » task
Priority: Normal » Critical
Status: Fixed » Active
Issue tags: +Needs change record

Needs change notice.

Nick_vh’s picture

http://drupal.org/node/1757426 Was created as change notification (no idea why it does not resolve it in some noce looking title)

Nick_vh’s picture

Status: Active » Fixed
Gábor Hojtsy’s picture

Title: Change notice: hook_search_preprocess needs to be language-specific » hook_search_preprocess needs to be language-specific
Category: task » feature
Priority: Critical » Normal
Issue tags: -Needs change record

Changing back.

@nick_vh: change notifications do not resolve with the [#number] format, that's it.

Gábor Hojtsy’s picture

Issue tags: +sprint

Should have had the sprint tag.

Gábor Hojtsy’s picture

Status: Fixed » Reviewed & tested by the community
FileSize
588 bytes

I think this is such a valuable addition that it deserves a changelog entry. It is pretty big for search :)

jhodgdon’s picture

Assigned: Unassigned » jhodgdon

Looks good, I'll get this documentation committed shortly.

jhodgdon’s picture

Assigned: jhodgdon » Unassigned
Status: Reviewed & tested by the community » Fixed

Changelog done... back to fixed!

Gábor Hojtsy’s picture

Issue tags: -sprint

Thanks all, finally all done! Moving off of sprint.

Status: Fixed » Closed (fixed)
Issue tags: -multilingual, -API change, -D8MI

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