Move search text processing to a service

Thanks for getting this going!

A few comments on the current patch:

a) In search.module... Why is there still a function body on search_invoke_preprocess()?

b) In the new interface:

+   * @see ::filterCharacters()

This is not correct coding standards, and I don't think the API module will make it into a link... It needs to at least be self:: .

c) Bikeshed coming... In the new interface/class, I don't really like the name of the filterCharacters() method. That name to me sounds like it is filtering out characters. But it can be doing a lot more than that:
- It invokes hook_search_preprocess(), which is what stemming modules do to reduce words to their stems (like walked/walking/walks all become walk). That to me is not a "character filter" but more of a "word-level preprocessing".
- CJK processing takes CJK characters and separates them into "words". Again this is kind of word-level processing

So maybe a better name would be preprocessText()?

Actually, we had an issue a while back to convert all of these preprocessing steps to a filter chain... let me find it.
#257007: Convert hook_search_preprocess() into a plugin

d) Actually this whole issue seems like a duplicate of #2719961: Create SearchText service to deprecate procedural code in search.module so one of them needs to be closed I think? I'll wait to continue review until then...

Patch in #2719961: Create SearchText service to deprecate procedural code in search.module slightly outdated but also converts "defines" (not sure why they are not constants)

Looks promising and have a change to get in before Alpha

I took a fresh look at the patch... A few thoughts/questions:

a) Are we still deprecating things in drupal 8 for removal in 9? I don't think so... At least it would need to be 8.9:

+ * @deprecated in drupal:8.8.0 and is removed from drupal:9.0.0. Use
+ *   \Drupal\search\SearchTextProcessor::PREG_CLASS_NUMBERS instead.

b)

+ * @deprecated in drupal:8.8.0 and is removed from drupal:9.0.0. Use
+ *   \Drupal\search\SearchTextProcessorInterface::analyse() instead.
+ *
+ * @see https://www.drupal.org/node/3078162
  * @see hook_search_preprocess()
  */
 function search_simplify($text, $langcode = NULL) {

analyse should be spelled the American way: analyze

But really... Looking at what the function does, I don't think analyze makes much sense as the function name anyway. It is being processed, not analyzed.

c) I don't understand this deprecation/error message:

+  @trigger_error('search_expand_cjk() is deprecated in drupal:8.8.0 and is removed from drupal:9.0.0. Use a custom implementation of SearchTextProcessorInterface instead. instead. See https://www.drupal.org/node/3078162', E_USER_DEPRECATED);
+  return \Drupal::service('search.text_processor')->expandCjk($matches);
 }

It says to use a custom implementation of SearchTextProcessorInterface, but then it calls a method on the core service?!? I guess because that function is marked as internal, but since it's an option on the Search module UI to do CJK processing, I think that we need it not to be internal?

d) nitpick -- on the interface:

+   * @see search_expand_cjk()
+   */
+  const PREG_CLASS_CJK =

Shouldn't have an @see to a deprecated function?

e) Similarly, the tests still talk about testing the search_simplify() function.

The deprecation possible starting from 9.1 for 10.0 removal iirc

Change record version fixed

Only addressed #19 a) and d)

Re #19 b), there is a relevant issue FYI, #2972224: Add .cspell.json to automate spellchecking in Drupal core,

Re #23, reroll is unnecessary unless we want a patch for 8.x.

The re-roll is for version

1. +++ b/core/modules/search/search.module
   @@ -10,9 +10,15 @@
    define('PREG_CLASS_NUMBERS',
   
   @@ -27,6 +33,11 @@
    define('PREG_CLASS_PUNCTUATION',
   
   @@ -58,7 +69,10 @@
    define('PREG_CLASS_CJK', '\x{1100}-\x{11FF}\x{3040}-\x{309F}\x{30A1}-\x{318E}' .
   

   Can we not define those values with the constants from Drupal\search\SearchTextProcessorInterface?

2. +++ b/core/modules/search/search.module
   @@ -10,9 +10,15 @@
   + *   \Drupal\search\SearchTextProcessor::PREG_CLASS_NUMBERS instead.
   
   @@ -27,6 +33,11 @@
   + *   \Drupal\search\SearchTextProcessor::PREG_CLASS_PUNCTUATION instead.
   
   @@ -58,7 +69,10 @@
   + *   \Drupal\search\SearchTextProcessor::PREG_CLASS_PUNCTUATION instead.
   

   Please use the Drupal\search\SearchTextProcessorInterface instead of Drupal\search\SearchTextProcessor

3. +++ b/core/modules/search/src/SearchIndex.php
   @@ -52,12 +59,15 @@ class SearchIndex implements SearchIndexInterface {
   +    $this->textProcessor = $text_processor;
   

   Here also needs deprecation stuff to happen.

4. +++ b/core/modules/search/tests/src/Functional/SearchSimplifyTest.php
   @@ -2,14 +2,15 @@
   -class SearchSimplifyTest extends BrowserTestBase {
   +class SearchSimplifyTest extends KernelTestBase {
   
   +++ b/core/modules/search/tests/src/Functional/SearchTokenizerTest.php
   @@ -2,14 +2,15 @@
   -class SearchTokenizerTest extends BrowserTestBase {
   +class SearchTokenizerTest extends KernelTestBase {
   

   This is out of scope for this issue. Create a followup for this.

5. +++ b/core/modules/search/search.module
   @@ -311,8 +240,14 @@ function _search_index_truncate(&$text) {
   + * @deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. Use a
   + *   custom implementation of SearchTextProcessor instead.
   ...
   +  @trigger_error('search_invoke_preprocess() is deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. Use a custom implementation of SearchTextProcessorInterface instead. See https://www.drupal.org/node/3078162', E_USER_DEPRECATED);
   

   There is no deprecation message testing for this @trigger_error().

6. When I do a code base search for "search_simplify(", I can still find it in documentation. Needs to be replaced.

7. +++ b/core/modules/search/src/SearchTextProcessorInterface.php
   @@ -0,0 +1,106 @@
   +   * @see search_expand_cjk()
   

   This is wrong. Please change it to Drupal\search\SearchTextProcessor::expandCjk().

@daffie about #27.1 - I also stuck to make it work with interface, looks define() is where autoload can't resolve

@andypost: When you cannot fix it, then skip it. Fix the other stuff and when it looks good I shall give it a RTBC. Then it is up to the committer to see what he or she thinks about it. I was only asking.

Fixes #27 (except 1, 4, 6)

+++ b/core/modules/search/src/SearchIndex.php
@@ -104,7 +118,7 @@ public function index($type, $sid, $langcode, $text, $update_weights = TRUE) {
-        list($tagname) = explode(' ', $value, 2);
+        [$tagname] = explode(' ', $value, 2);

out of scope - fixed

Fix #27.6 so the only test conversion follow-up or blocker left?

Here I have changed all occurrences of "analyse" to "analyze".

@ravi.shankar: The change from analyse to analyze look good. Could you also undo the following change (#27.4):

+++ b/core/modules/search/tests/src/Functional/SearchSimplifyTest.php
@@ -2,14 +2,15 @@
-class SearchSimplifyTest extends BrowserTestBase {
+class SearchSimplifyTest extends KernelTestBase {

+++ b/core/modules/search/tests/src/Functional/SearchTokenizerTest.php
@@ -2,14 +2,15 @@
-class SearchTokenizerTest extends BrowserTestBase {
+class SearchTokenizerTest extends KernelTestBase {

I will create a followup for it.

Created the followup: #3134333: Change Drupal\Tests\search\Functional\SearchSimplifyTest and Drupal\Tests\search\Functional\SearchTokenizerTest to kerneltests.

Back to needs work for #27.4.

Here I have addressed comment #35.

I am working on this.

Added re-roll of patch #38.

Back to rtbc

1. +++ b/core/modules/search/search.module
   @@ -13,6 +13,11 @@
    define('PREG_CLASS_NUMBERS',
      '\x{30}-\x{39}\x{b2}\x{b3}\x{b9}\x{bc}-\x{be}\x{660}-\x{669}\x{6f0}-\x{6f9}' .
   
   @@ -27,6 +32,12 @@
    define('PREG_CLASS_PUNCTUATION',
      '\x{21}-\x{23}\x{25}-\x{2a}\x{2c}-\x{2f}\x{3a}\x{3b}\x{3f}\x{40}\x{5b}-\x{5d}' .
   
   @@ -58,7 +69,10 @@
    define('PREG_CLASS_CJK', '\x{1100}-\x{11FF}\x{3040}-\x{309F}\x{30A1}-\x{318E}' .
      '\x{31A0}-\x{31B7}\x{31F0}-\x{31FF}\x{3400}-\x{4DBF}\x{4E00}-\x{9FCF}' .
   

   We can change these to define('PREG_CLASS_NUMBERS', SearchTextProcessorInterface::PREG_CLASS_NUMBERS);
   etc... that way these big constants are only defined in a single location.

   This does work...

   >>> define('PREG_CLASS_NUMBERS_BLAH', \Drupal\search\SearchTextProcessorInterface::PREG_CLASS_NUMBERS);
   => true
   >>> PREG_CLASS_NUMBERS_BLAH
   => "\x{30}-\x{39}\x{b2}\x{b3}\x{b9}\x{bc}-\x{be}\x{660}-\x{669}\x{6f0}-\x{6f9}\x{966}-\x{96f}\x{9e6}-\x{9ef}\x{9f4}-\x{9f9}\x{a66}-\x{a6f}\x{ae6}-\x{aef}\x{b66}-\x{b6f}\x{be7}-\x{bf2}\x{c66}-\x{c6f}\x{ce6}-\x{cef}\x{d66}-\x{d6f}\x{e50}-\x{e59}\x{ed0}-\x{ed9}\x{f20}-\x{f33}\x{1040}-\x{1049}\x{1369}-\x{137c}\x{16ee}-\x{16f0}\x{17e0}-\x{17e9}\x{17f0}-\x{17f9}\x{1810}-\x{1819}\x{1946}-\x{194f}\x{2070}\x{2074}-\x{2079}\x{2080}-\x{2089}\x{2153}-\x{2183}\x{2460}-\x{249b}\x{24ea}-\x{24ff}\x{2776}-\x{2793}\x{3007}\x{3021}-\x{3029}\x{3038}-\x{303a}\x{3192}-\x{3195}\x{3220}-\x{3229}\x{3251}-\x{325f}\x{3280}-\x{3289}\x{32b1}-\x{32bf}\x{ff10}-\x{ff19}"
   

2. +++ b/core/modules/search/src/SearchTextProcessor.php
   @@ -0,0 +1,198 @@
   +   * @internal
   +   */
   +  public function expandCjk(array $matches) {
   

   How about making this protected and deprecating the original code but with no replacement? And not emptying out the old code? There are no contrib usages of this method.

3. +++ b/core/modules/search/tests/src/Kernel/SearchDeprecationTest.php
   @@ -0,0 +1,63 @@
   +    $this->assertNotEmpty(search_index_split("two words"));
   

   Let's assert it equals ['two', 'words'] - that way we test at least a little that we've not broken the implementation.

4. +++ b/core/modules/search/tests/src/Kernel/SearchDeprecationTest.php
   @@ -0,0 +1,63 @@
   +    $this->assertNotEmpty(search_simplify("subdermatoglyphic"));
   

   Again let's test the output value but the input should also be something that would change - maybe something like Vögel as that would be changed to vogel.

5. +++ b/core/modules/search/tests/src/Kernel/SearchDeprecationTest.php
   @@ -0,0 +1,63 @@
   +    $this->assertNotEmpty(search_expand_cjk(["이런"]));
   

   We should be testing the output here too... at the very least it should we could assert the output is the input wrapped in spaces.

On it

@kim.pepper Hi yes you are correct.Please find the new patch without #41.1

RE#41.1 :#45 failed due to the #41.1 change so i am revert this point only.
regards
Pavnish

Let's see if this change fixes the testbot.

Blocker commited

Remove the fix from #50

All points of @alexpott are addressed.
Back to RTBC.

1. +++ b/core/modules/search/search.module
   @@ -224,31 +173,15 @@ function search_simplify($text, $langcode = NULL) {
    function search_expand_cjk($matches) {
   -  $min = \Drupal::config('search.settings')->get('index.minimum_word_size');
   -  $str = $matches[0];
   -  $length = mb_strlen($str);
   -  // If the text is shorter than the minimum word size, don't tokenize it.
   -  if ($length <= $min) {
   -    return ' ' . $str . ' ';
   -  }
   -  $tokens = ' ';
   -  // Build a FIFO queue of characters.
   -  $chars = [];
   -  for ($i = 0; $i < $length; $i++) {
   -    // Add the next character off the beginning of the string to the queue.
   -    $current = mb_substr($str, 0, 1);
   -    $str = substr($str, strlen($current));
   -    $chars[] = $current;
   -    if ($i >= $min - 1) {
   -      // Make a token of $min characters, and add it to the token string.
   -      $tokens .= implode('', $chars) . ' ';
   -      // Shift out the first character in the queue.
   -      array_shift($chars);
   -    }
   -  }
   -  return $tokens;
   +  @trigger_error('search_expand_cjk() is deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. Use a custom implementation of SearchTextProcessorInterface instead. instead. See https://www.drupal.org/node/3078162', E_USER_DEPRECATED);
   +  return \Drupal::service('search.text_processor')->expandCjk($matches);
   

   I think expandCjk should be protected. Marking it @internal seems only half the job. How about leaving this code here and only deprecating it. Then we wouldn't need to call into the method and it'd no longer need to be public but not on the interface. This is what we did with search_invoke_preprocess() and imo that makes sense.

2. +++ b/core/modules/search/src/SearchTextProcessor.php
   @@ -0,0 +1,198 @@
   +  public function process($text, $langcode = NULL) {
   ...
   +  public function analyze($text, $langcode = NULL) {
   ...
   +  protected function invokePreprocess(&$text, $langcode = NULL) {
   ...
   +  public function expandCjk(array $matches) {
   ...
   +  protected function truncate(&$text) {
   
   +++ b/core/modules/search/src/SearchTextProcessorInterface.php
   @@ -0,0 +1,106 @@
   +  public function process($text, $langcode = NULL);
   ...
   +  public function analyze($text, $langcode = NULL);
   

   Let's add typehints for scalars ie string $text and also add return typehints. As this is new code this means we don't have to do it later.

3. +++ b/core/modules/search/src/SearchTextProcessor.php
   @@ -0,0 +1,198 @@
   +    // The same words may often be passed in during excerpt generation.
   +    static $truncated = [];
   +    if (isset($truncated[$text])) {
   +      $text = $truncated[$text];
   +      return;
   +    }
   +
   +    // If we didn't find it in the static array, perform the operation.
   +    $original = $text;
   +    if (is_numeric($text)) {
   +      $text = ltrim($text, '0');
   +    }
   +    $text = Unicode::truncate($text, 50);
   +    // Save it for the next time.
   +    $truncated[$original] = $text;
   +  }
   

   This could be a class property and that way we'd have one less static and it'd be cleared on container rebuild - which makes sense to me.

   But also I think we should rewrite this. Because calling Unicode::truncate with none of the options set it wasteful.

   This method should look like this:

     protected function truncate(string &$text): void {
       if (mb_strlen($text) <= 50) {
         return;
       }
       if (is_numeric($text)) {
         $text = ltrim($text, '0');
       }
       $text = mb_substr($text, 0, 50);
     }
   

   I severely doubt that that static cache is worth if we check the word length and return early. Also this approach will benefit memory because at the moment the static $truncated is going to be full of 'the' => 'the, 'of' => 'of' etc...

   This used to relevant when we did not have mb support baked in. But now we use Symfony's mb_* fallback and everyone has this enabled anyway this sort of optimisation is wasteful.

Great! all points addressed

Committed 640ea36 and pushed to 9.1.x. Thanks!

  public function testDeprecatedSimplify() {
    // cSpell:disable-next-line
    $this->assertEquals("vogel", search_simplify("Vögel"));
  }

Added the // cSpell:disable-next-line on commit because it seemed sensible and avoids cSpell errorring on this test.

+++ b/core/modules/search/src/SearchTextProcessor.php
@@ -0,0 +1,187 @@
+  protected function truncate(string &$text): void {
+    if (is_numeric($text)) {
+      $text = ltrim($text, '0');
+    }
+    if (mb_strlen($text) <= 50) {
+      return;
+    }
+    $text = mb_substr($text, 0, 50);
+  }

I was wondering whether doing the string length check first was worth it - and it looks as though it is - https://3v4l.org/MVdrk

@alexpott Thanks!

Looks #257007: Convert hook_search_preprocess() into a plugin could be closed as outdated now

The remaining issue is #2552497: [PP-1] Convert search_excerpt() to a filter