[NodeSearchBundle][SearchBundle] Multi domain/language search population fix #1635

cv65kr · 2017-11-06T14:19:13Z

Q	A
Bug fix?	yes
New feature?	yes
BC breaks?	no
Deprecations?	no
Fixed tickets	#1246

Now we can use language names like "en", "en_US", "en_us", also fixes error mentioned in #1246 ticket.

--edited by numkil
Also with new feature to check for unavailable language analyzers and prompt the user.

sandergo90 · 2017-11-15T15:13:14Z

src/Kunstmaan/NodeSearchBundle/Configuration/NodePagesConfiguration.php

-            $localeAnalysis = clone($analysis);
-            $language = $this->analyzerLanguages[$locale]['analyzer'];
+            // Multilanguage check
+            if ( count(preg_grep('/[a-z]{2}_?+[a-zA-Z]{2}/', [$locale])) > 0 ) {


@cv65kr can you remove the space before "count" ?

@sandergo90 sure, fixed.

Numkil

Can you sqaush your commits to 1 commit ? I will be testing your PR later this evening :)

Numkil · 2017-11-15T15:29:44Z

src/Kunstmaan/NodeSearchBundle/Configuration/NodePagesConfiguration.php

@@ -318,7 +326,7 @@ public function setAnalysis(Index $index, AnalysisFactoryInterface $analysis)
     *
     * @return Mapping
     */
-    protected function createDefaultSearchFieldsMapping(Index $index, $lang = 'en')
+    protected function createDefaultSearchFieldsMapping(Index $index)


Can you undo this change? I know it looks stupid but this is necessary to avoid BC breaks on older projects that still require $lang in this method.

Numkil · 2017-11-15T15:29:59Z

src/Kunstmaan/NodeSearchBundle/Configuration/NodePagesConfiguration.php

@@ -334,7 +342,7 @@ protected function createDefaultSearchFieldsMapping(Index $index, $lang = 'en')
     * @param Index  $index
     * @param string $lang
     */
-    protected function setMapping(Index $index, $lang = 'en')
+    protected function setMapping(Index $index, $lang)


Can you undo this change? I know it looks stupid but this is necessary to avoid BC breaks on older projects that still require $lang in this method.

Numkil · 2017-11-15T15:30:24Z

src/Kunstmaan/SearchBundle/Search/Search.php

@@ -77,6 +77,8 @@ public function getClient()
     */
    public function createDocument($uid, $document, $indexName = '', $indexType = '')
    {
+        $indexName = strtolower($indexName);


Why are you doing strtolower here?

Numkil · 2017-11-15T16:41:52Z

hey @cv65kr you seem to have made a mistake in your rebase/squash procedure and your commit now contains commits from other people not in this branch yet.

cv65kr · 2017-11-15T17:15:30Z

@Numkil I saw that, but i had to leave ;) i will try to fix that today

Numkil · 2017-11-15T17:34:58Z

No hurries I will review it when you are finished.

cv65kr · 2017-11-15T18:02:44Z

@Numkil done.
Going back to your question about strtolower, letters must be lowercase because elastic return exception if index name have a uppercase letters. Check out on my screenshot :)

Numkil · 2017-11-17T09:08:26Z

src/Kunstmaan/NodeSearchBundle/Configuration/NodePagesConfiguration.php

-            $localeAnalysis = clone($analysis);
-            $language = $this->analyzerLanguages[$locale]['analyzer'];
+            // Multilanguage check
+            if (count(preg_grep('/[a-z]{2}_?+[a-zA-Z]{2}/', [$locale])) > 0) {


Why not use preg_match instead?

Numkil · 2017-11-17T09:09:37Z

src/Kunstmaan/NodeSearchBundle/Configuration/NodePagesConfiguration.php

+                // Create index with analysis
+                $this->setAnalysis($index, $localeAnalysis->setupLanguage($language));
+            } else {
+                $index->create([]);


I would prefer a very clear warning happening here that says something like,
"hey we did not crash but something is wrong here, we could not set up any analysis for this language".

@Numkil, You mean exception?

Numkil · 2017-11-17T10:33:51Z

src/Kunstmaan/NodeSearchBundle/Configuration/NodePagesConfiguration.php

+                // Create index with analysis
+                $this->setAnalysis($index, $localeAnalysis->setupLanguage($language));
+            } else {
+                $index->create([]);


I think the best course of action is to somehow show a warning in the command output. Otherwise it will not be clear for developers why their search might not be behaving as expected without manually checking the index metadata. If a simple message is shown in the console command output then at least users will immediately know that they forgot to define an analyzer language

cv65kr · 2017-11-17T14:02:17Z

@Numkil something like that?

Numkil · 2017-11-17T14:29:04Z

@cv65kr yes this is awesome! I saw one little typo where you typed answear instead of answer but if you fix that I will merge this PR tonight or tomorrow. Thank you for your help and quick responses!

cv65kr · 2017-11-17T14:41:04Z

@Numkil ohhh, already fixed :)

Numkil · 2017-11-18T10:15:43Z

@cv65kr thanks for the contribution.

cv65kr · 2017-11-18T10:49:40Z

@Numkil, no problem :) You can close #1246 because its solved by this PR.

…pport bundles 3.6 websites on servers with ES6 (#1877) * [NodeSearchBundle]: fix request scope * [NodeSearchBundle] Make NodeSearcher properties protected for extending * [NodeSearchBundle]: add possibility to index multiple regions [NodeSearchBundle]: make contexts default array, and merge default * [KunstmaanNodeSearchBundle]: upgrade elastica to 5.1 Change ruflin elastica version [KunstmaanNodeSearchBundle]: upgrade elastica to 5.1 Add changelog * move boost from mapping to query + allow text as type for elasticsearch 5.0 * [NodeSearchBundle] Bugfix for indexing StructuredNodes and children (#1550) [NodeSearchBundle] Bugfix for indexing StructuredNodes and children * [NodeSearchBundle] Bugfix for indexing StructuredNodes and children * [NodeSearchBundle][SearchBundle] Multi domain/language search population fix (#1635) * squash commits * Analyzer info in command * Typo fix * typo * [NodeSearchBundle]: check if we have html * [NodeSearchBundle] shards and replicas configurable [NodeSearchBundle] default values shards/replicas * [SearchBundle][FIX] Populate command failing * [SearchBundle] Convert analyzer languages config to lowercase to prevent config mismatch because of case sensitivity * [SearchBundle] SetupIndex command failing when overwriting nodeconfiguration with own file. * [SearchBundle][FIX] SetupIndex command failing * [SearchBundle][FIX] SetupIndex command failing * [SearchBundle][FIX] SetupIndex command failing * [SearchBundle] add punctuation to kuma_ngram tokenizer * [NodeSearchBundle] added extra properties for elasticsearch mapping (#1794) * [KunstmaanNodeSearchBundle] add keyword as data type (#1793) [KunstmaanNodeSearchBundle]: change data types Revert keyword to string for compability with 2.4 * [NodeSearchBundle] Elastic search version 6 support * [NodeSearchBundle] Elastic search version 6 support * [NodeSearchBundle] backport fixes * [allbundles] bump phpversions

* [SeoBundle]: change meta length (#1520) [SeoBundle]: change translations * [SeoBundle] Refactor seo description to be longer according to google's new length (#1797) * [NodeSearchBundle] [SearchBundle] ElasticSearch Backport so we can support bundles 3.6 websites on servers with ES6 (#1877) * [NodeSearchBundle]: fix request scope * [NodeSearchBundle] Make NodeSearcher properties protected for extending * [NodeSearchBundle]: add possibility to index multiple regions [NodeSearchBundle]: make contexts default array, and merge default * [KunstmaanNodeSearchBundle]: upgrade elastica to 5.1 Change ruflin elastica version [KunstmaanNodeSearchBundle]: upgrade elastica to 5.1 Add changelog * move boost from mapping to query + allow text as type for elasticsearch 5.0 * [NodeSearchBundle] Bugfix for indexing StructuredNodes and children (#1550) [NodeSearchBundle] Bugfix for indexing StructuredNodes and children * [NodeSearchBundle] Bugfix for indexing StructuredNodes and children * [NodeSearchBundle][SearchBundle] Multi domain/language search population fix (#1635) * squash commits * Analyzer info in command * Typo fix * typo * [NodeSearchBundle]: check if we have html * [NodeSearchBundle] shards and replicas configurable [NodeSearchBundle] default values shards/replicas * [SearchBundle][FIX] Populate command failing * [SearchBundle] Convert analyzer languages config to lowercase to prevent config mismatch because of case sensitivity * [SearchBundle] SetupIndex command failing when overwriting nodeconfiguration with own file. * [SearchBundle][FIX] SetupIndex command failing * [SearchBundle][FIX] SetupIndex command failing * [SearchBundle][FIX] SetupIndex command failing * [SearchBundle] add punctuation to kuma_ngram tokenizer * [NodeSearchBundle] added extra properties for elasticsearch mapping (#1794) * [KunstmaanNodeSearchBundle] add keyword as data type (#1793) [KunstmaanNodeSearchBundle]: change data types Revert keyword to string for compability with 2.4 * [NodeSearchBundle] Elastic search version 6 support * [NodeSearchBundle] Elastic search version 6 support * [NodeSearchBundle] backport fixes * [allbundles] bump phpversions * update changelog * [AdminlistBundle] fixed type from string to array (#1925)

…#1936) * [SeoBundle]: change meta length (Kunstmaan#1520) [SeoBundle]: change translations * [SeoBundle] Refactor seo description to be longer according to google's new length (Kunstmaan#1797) * [NodeSearchBundle] [SearchBundle] ElasticSearch Backport so we can support bundles 3.6 websites on servers with ES6 (Kunstmaan#1877) * [NodeSearchBundle]: fix request scope * [NodeSearchBundle] Make NodeSearcher properties protected for extending * [NodeSearchBundle]: add possibility to index multiple regions [NodeSearchBundle]: make contexts default array, and merge default * [KunstmaanNodeSearchBundle]: upgrade elastica to 5.1 Change ruflin elastica version [KunstmaanNodeSearchBundle]: upgrade elastica to 5.1 Add changelog * move boost from mapping to query + allow text as type for elasticsearch 5.0 * [NodeSearchBundle] Bugfix for indexing StructuredNodes and children (Kunstmaan#1550) [NodeSearchBundle] Bugfix for indexing StructuredNodes and children * [NodeSearchBundle] Bugfix for indexing StructuredNodes and children * [NodeSearchBundle][SearchBundle] Multi domain/language search population fix (Kunstmaan#1635) * squash commits * Analyzer info in command * Typo fix * typo * [NodeSearchBundle]: check if we have html * [NodeSearchBundle] shards and replicas configurable [NodeSearchBundle] default values shards/replicas * [SearchBundle][FIX] Populate command failing * [SearchBundle] Convert analyzer languages config to lowercase to prevent config mismatch because of case sensitivity * [SearchBundle] SetupIndex command failing when overwriting nodeconfiguration with own file. * [SearchBundle][FIX] SetupIndex command failing * [SearchBundle][FIX] SetupIndex command failing * [SearchBundle][FIX] SetupIndex command failing * [SearchBundle] add punctuation to kuma_ngram tokenizer * [NodeSearchBundle] added extra properties for elasticsearch mapping (Kunstmaan#1794) * [KunstmaanNodeSearchBundle] add keyword as data type (Kunstmaan#1793) [KunstmaanNodeSearchBundle]: change data types Revert keyword to string for compability with 2.4 * [NodeSearchBundle] Elastic search version 6 support * [NodeSearchBundle] Elastic search version 6 support * [NodeSearchBundle] backport fixes * [allbundles] bump phpversions * update changelog * [AdminlistBundle] fixed type from string to array (Kunstmaan#1925)

Numkil self-assigned this Nov 15, 2017

sandergo90 reviewed Nov 15, 2017

View reviewed changes

Numkil reviewed Nov 15, 2017

View reviewed changes

cv65kr force-pushed the search-population-fix branch from cd0584f to fccc38b Compare November 15, 2017 15:41

cv65kr force-pushed the search-population-fix branch 2 times, most recently from cd0584f to 8cae086 Compare November 15, 2017 17:31

cv65kr closed this Nov 15, 2017

cv65kr force-pushed the search-population-fix branch from 290be4b to d567167 Compare November 15, 2017 17:50

squash commits

0cd47b7

cv65kr reopened this Nov 15, 2017

Numkil reviewed Nov 17, 2017

View reviewed changes

Analyzer info in command

31c6cbd

Typo fix

76d0e0f

typo

315dee6

Numkil changed the base branch from 5.0 to 4.0 November 18, 2017 10:09

Numkil changed the base branch from 4.0 to 5.0 November 18, 2017 10:12

Numkil merged commit 2f4fd20 into Kunstmaan:5.0 Nov 18, 2017

sandergo90 added this to the 5.0.0 milestone Nov 18, 2017

sandergo90 added Profile: Backend Type: Feature labels Nov 18, 2017

Numkil mentioned this pull request Nov 18, 2017

Multi domain/language search population/results error. #1246

Closed

cv65kr mentioned this pull request Nov 25, 2017

[SearchBundle] Populate command failing #1704

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NodeSearchBundle][SearchBundle] Multi domain/language search population fix #1635

[NodeSearchBundle][SearchBundle] Multi domain/language search population fix #1635

cv65kr commented Nov 6, 2017 •

edited by Numkil

sandergo90 Nov 15, 2017

cv65kr Nov 15, 2017

Numkil left a comment

Numkil Nov 15, 2017

Numkil Nov 15, 2017

Numkil Nov 15, 2017

Numkil commented Nov 15, 2017

cv65kr commented Nov 15, 2017

Numkil commented Nov 15, 2017

cv65kr commented Nov 15, 2017

Numkil Nov 17, 2017

cv65kr Nov 17, 2017

Numkil Nov 17, 2017

cv65kr Nov 17, 2017

Numkil Nov 17, 2017

cv65kr commented Nov 17, 2017

Numkil commented Nov 17, 2017

cv65kr commented Nov 17, 2017

Numkil commented Nov 18, 2017

cv65kr commented Nov 18, 2017

[NodeSearchBundle][SearchBundle] Multi domain/language search population fix #1635

[NodeSearchBundle][SearchBundle] Multi domain/language search population fix #1635

Conversation

cv65kr commented Nov 6, 2017 • edited by Numkil

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Numkil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Numkil commented Nov 15, 2017

cv65kr commented Nov 15, 2017

Numkil commented Nov 15, 2017

cv65kr commented Nov 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cv65kr commented Nov 17, 2017

Numkil commented Nov 17, 2017

cv65kr commented Nov 17, 2017

Numkil commented Nov 18, 2017

cv65kr commented Nov 18, 2017

cv65kr commented Nov 6, 2017 •

edited by Numkil