EZP-31299: Added 'search in specific language' feature #1223

michal-myszka · 2020-02-04T13:07:21Z

Question	Answer
Tickets	https://jira.ez.no/browse/EZP-31299
Requires	ezsystems/ezpublish-kernel#2940
Bug fix?	yes
New feature?	yes
BC breaks?	no
Tests pass?	yes
Doc needed?	yes
License	GPL-2.0

Added 'search in specific language' feature.
This allows to search also in content translations, in one of the selected search language. The "Use always available" option is ALWAYS ENABLED here.

There are two ways of processing search, dependent on chosen search engine.

Legacy Search SQL Engine
Searching returns always "distinct" results, so one content-object couldn't be repeated in different languages (unlike in Solr engine). Results display translation is based on chosen search language or if this translation doesn't exist then the result will be present in the "locale language".

Solr Search Engine
Searching results aren't always "distinct" if the query string is provided, so one content-object could be repeated in different languages on the list (unlike LSSE) (useAlwaysAvailable and new excludeTranslationsFromAlwaysAvailable options). Results display translation is based on matched translation. If query string is omitted, then the results will be "distinct" (useAlwaysAvailable option).

Added dropdown in search form, if there is more than 1 language configured
Added new choice loader ConfiguredLanguagesChoiceLoader and ConfiguredLanguagesChoiceType for providing also non-available languages

! Required changes from ezsystems/ezpublish-kernel#2940

Checklist:

Coding standards ($ composer fix-cs)
Test results on legacy search engine
Styling and scripting (front)
Prepare migration
Prepare SQL for fill for current data
Update tests
Merge first EZP-31299: Added language mask to LSE search index ezpublish-kernel#2940
Ready for Code Review

*Screens for LSSE. One more screen is in the comment with language choose popup when click on edit button from results list.

…uage"

…form (to chosen translation)

…into ezp-31299-additional-search-languages-filter

src/bundle/Resources/views/admin/search/search.html.twig

adamwojs · 2020-02-18T09:40:10Z

src/lib/Form/Type/Language/ConfiguredLanguagesChoiceType.php

+     */
+    public function __construct(ConfiguredLanguagesChoiceLoader $languageChoiceLoader)
+    {
+        $this->languageChoiceLoader = $languageChoiceLoader;


Suggested change

$this->languageChoiceLoader = $languageChoiceLoader;

parent::__construct($languageChoiceLoader);

adamwojs · 2020-02-18T09:40:24Z

src/lib/Form/Type/Language/LanguageChoiceType.php

 {
-    /** @var \EzSystems\EzPlatformAdminUi\Form\Type\ChoiceList\Loader\LanguageChoiceLoader */
-    private $languageChoiceLoader;
-
    /**
     * @param \EzSystems\EzPlatformAdminUi\Form\Type\ChoiceList\Loader\LanguageChoiceLoader $languageChoiceLoader
     */
    public function __construct(LanguageChoiceLoader $languageChoiceLoader)
    {
        $this->languageChoiceLoader = $languageChoiceLoader;


Suggested change

$this->languageChoiceLoader = $languageChoiceLoader;

parent::__construct($languageChoiceLoader);

adamwojs · 2020-02-18T09:41:54Z

src/lib/Form/Type/Language/LanguageChoiceTypeAbstract.php

+use Symfony\Component\Form\Extension\Core\Type\ChoiceType;
+use Symfony\Component\OptionsResolver\OptionsResolver;
+
+abstract class LanguageChoiceTypeAbstract extends AbstractType


We usually use Abstract prefix instead of suffix in case of abstract classes.

src/bundle/Resources/config/services/forms.yml

src/bundle/Controller/SearchController.php

src/bundle/Resources/config/services/forms.yml

src/bundle/Resources/views/admin/search/list.html.twig

src/lib/Form/Type/ChoiceList/Loader/ConfiguredLanguagesChoiceLoader.php

src/lib/Form/Type/ChoiceList/Loader/LanguageChoiceLoader.php

alongosz · 2020-02-18T10:47:05Z

src/lib/Form/Type/Language/ConfiguredLanguagesChoiceType.php

+/**
+ * Form Type allowing to select from all configured (also not enabled) Languages.
+ */
+class ConfiguredLanguagesChoiceType extends LanguageChoiceTypeAbstract


The implementation of ConfiguredLanguagesChoiceType and LanguageChoiceType is almost identical, so it feels redundant. How does it happen that for ConfiguredLanguagesChoiceType the ConfiguredLanguagesChoiceLoader is injected instead? Based on the usage I don't see it (expected at least choice_loader option in \EzSystems\EzPlatformAdminUi\Form\Type\Search\SearchType::buildForm).

ConfiguredLanguagesChoiceType and LanguageChoiceType have injected different ChoiceLoaders, with a little different strategy. Above classes are autowiring by typehint.

Above classes are autowiring by typehint.

Where exactly does the auto-wiring for that occur? The proper solution in this case is not to auto-wire, but inject directly the specific implementation (so single constructor in abstract class only and direct injection of LanguageChoiceLoader and ConfiguredLanguagesChoiceLoader). Still, can't find a place where it should happen. Form type is used as <ClassName>::class so seems like constructor is unused.

src/lib/Tab/Dashboard/PagerContentToDataMapper.php

Co-Authored-By: Andrew Longosz <alongosz@users.noreply.github.com>

…choose after click the edit button on the search results

dew326 · 2020-02-20T11:27:09Z

Frontend part updated:

@GrabowskiM @lucasOsti please review the last commit (7e6de36).

/cc @lserwatka

src/bundle/Resources/public/js/scripts/button.translation.edit.js

…rch results and refactor PagerContentToDataMapper

adamwojs · 2020-02-24T10:18:34Z

src/lib/Search/PagerContentToDataMapperAbstract.php

+
+    /**
+     * @param array $data
+     * @param array $contentTypeIds


Suggested change

* @param array $contentTypeIds

* @param int[] $contentTypeIds

adamwojs · 2020-02-24T10:19:14Z

src/lib/Search/PagerSearchContentToDataMapper.php

+class PagerSearchContentToDataMapper extends PagerContentToDataMapperAbstract
+{
+    /**
+     * @param Pagerfanta $pager


Suggested change

* @param Pagerfanta $pager

* @param \Pagerfanta\Pagerfanta $pager

src/lib/Tab/Dashboard/PagerContentToDataMapper.php

adamwojs · 2020-02-24T10:37:40Z

src/lib/Form/Type/ChoiceList/Loader/ConfiguredLanguagesChoiceLoader.php

+    public function __construct(LanguageService $languageService, ConfigResolverInterface $configResolver)
+    {
+        $this->languageService = $languageService;
+        $this->siteAccessLanguages = $configResolver->getParameter('languages');


$configResolver->getParameter('languages') should be called in https://github.com/ezsystems/ezplatform-admin-ui/pull/1223/files#diff-5f4e37227ab6d0e555d4b6703d406632R96

adamwojs · 2020-02-24T10:40:00Z

src/lib/Search/PagerContentToDataMapper.php

+class PagerContentToDataMapper extends PagerContentToDataMapperAbstract
+{
+    /**
+     * @param Pagerfanta $pager


Suggested change

* @param Pagerfanta $pager

* @param \Pagerfanta\Pagerfanta $pager

adamwojs · 2020-02-24T10:41:07Z

src/lib/Search/PagerContentToDataMapper.php

+
+        foreach ($pager as $content) {
+            /** @var \eZ\Publish\API\Repository\Values\Content\Content $content */
+            $contentInfo = $content->getVersionInfo()->getContentInfo();


$content->getVersionInfo()->getContentInfo() could be simplified to: $content->contentInfo

adamwojs · 2020-02-24T10:44:58Z

src/lib/Search/PagerContentToDataMapperAbstract.php

+            $contentType = isset($contentTypes[$item['contentTypeId']])
+                ? $contentTypes[$item['contentTypeId']]
+                : $item['content']->getContentType();


It could be simplified to $contentTypes[$item['contentTypeId']] ?? $item['content']->getContentType(). To improve readability even more you can all extract $item['contentTypeId'] to variable.

alongosz · 2020-02-24T10:10:44Z

src/bundle/Resources/views/parts/edit_translation_button.html.twig

+            </div>
+        </div>
+    </div>
+{% endif %}


missing line feed

src/lib/Form/Type/ChoiceList/Loader/ConfiguredLanguagesChoiceLoader.php

alongosz · 2020-02-24T10:26:21Z

src/lib/Form/Type/Language/ConfiguredLanguagesChoiceType.php

+    /**
+     * @param \EzSystems\EzPlatformAdminUi\Form\Type\ChoiceList\Loader\ConfiguredLanguagesChoiceLoader $languageChoiceLoader
+     */


This PHPDoc block is not really needed, the prototype provides all the necessary information.

There is more accurate information (prototype type-hint is only ChoiceLoaderInterface, because inheriting classes have different implementations)

alongosz · 2020-02-24T10:38:51Z

src/lib/Search/PagerContentToDataMapperAbstract.php

+    /**
+     * @param \eZ\Publish\API\Repository\ContentTypeService $contentTypeService
+     * @param \eZ\Publish\API\Repository\UserService $userService
+     * @param \eZ\Publish\Core\MVC\Symfony\Locale\UserLanguagePreferenceProviderInterface $userLanguagePreferenceProvider
+     * @param \eZ\Publish\Core\Helper\TranslationHelper $translationHelper
+     * @param \eZ\Publish\API\Repository\LanguageService $languageService
+     */


This can be skipped as doesn't provide anything more than the prototype (note: we've recently decided to skip that, so we'll find such cases in the codebase).

On the other hand it goes to 1.5 so ¯\(ツ)/¯

alongosz · 2020-02-24T12:09:25Z

src/lib/Form/Type/Language/ConfiguredLanguagesChoiceType.php

+/**
+ * Form Type allowing to select from all configured (also not enabled) Languages.
+ */
+class ConfiguredLanguagesChoiceType extends LanguageChoiceTypeAbstract


Above classes are autowiring by typehint.

Where exactly does the auto-wiring for that occur? The proper solution in this case is not to auto-wire, but inject directly the specific implementation (so single constructor in abstract class only and direct injection of LanguageChoiceLoader and ConfiguredLanguagesChoiceLoader). Still, can't find a place where it should happen. Form type is used as <ClassName>::class so seems like constructor is unused.

ViniTou · 2020-02-24T12:10:52Z

src/lib/Search/PagerContentToDataMapperAbstract.php

+use EzSystems\EzPlatformAdminUi\Specification\ContentIsUser;
+use EzSystems\EzPlatformAdminUi\Specification\UserExists;
+
+class PagerContentToDataMapperAbstract


Is this class abstract or not?

Yes, it is. Corrected

ViniTou · 2020-02-24T12:12:31Z

src/lib/Search/PagerContentToDataMapperAbstract.php

+     * @param array $data
+     * @param array $contentTypeIds
+     */
+    protected function setTranslatedContentTypesNames(array &$data, array $contentTypeIds): void


not rly fan of passing array by reference, but if no one protests ;)

I understand :) also not a rly fan of this method, but it's only moved from another place with little refactor (not more time in this task)

michal-myszka · 2020-02-24T13:07:23Z

Above classes are autowiring by typehint.

Where exactly does the auto-wiring for that occur? The proper solution in this case is not to auto-wire, but inject directly the specific implementation (so single constructor in abstract class only and direct injection of LanguageChoiceLoader and ConfiguredLanguagesChoiceLoader). Still, can't find a place where it should happen. Form type is used as ::class so seems like constructor is unused.

@alongosz the logic for getting service starts here: \Symfony\Component\Form\Extension\DependencyInjection\DependencyInjectionExtension::getType()

The constructors are used and I've verified, that they receive the correct dependency implementations.

alongosz · 2020-02-24T14:17:33Z

the logic for getting service starts here: \Symfony\Component\Form\Extension\DependencyInjection\DependencyInjectionExtension::getType()

The constructors are used and I've verified, that they receive the correct dependency implementations.

Given the amount of lines of code which was required for that auto-wiring to work and the static analysis issues which this led to, I would say that the best approach is to keep one LanguageType and configure proper Choice Loaders when building forms.

POV ping @webhdx

src/lib/Tab/Dashboard/PagerContentToDataMapper.php

webhdx · 2020-02-24T14:14:28Z

src/lib/Form/Type/Language/LanguageChoiceType.php

    /**
     * @param \EzSystems\EzPlatformAdminUi\Form\Type\ChoiceList\Loader\LanguageChoiceLoader $languageChoiceLoader
     */
    public function __construct(LanguageChoiceLoader $languageChoiceLoader)
    {
-        $this->languageChoiceLoader = $languageChoiceLoader;
-    }
-


What you are doing here is wrong. You don't need AbstractLanguageChoiceType. Since you only need to change choice loader in your types, then use choice_loader option. Do not create X additional classes to achieve what is already handled well by Symfony.

$formBuilder->add('configured_language_choice_type', LanguageChoiceType::class, [ 'choice_loader' => new ConfiguredLanguagesChoiceLoader(), // or inject from DIC ];

Please check, that I'm using the choice_loader option (see in AbstractLanguageChoiceType). I considered two options - your approach and current implementation, bu I've finally decided, that current solution may be better. I want to create X additional classes, as you suggest, but I think these two field types are useful and convenient to use - when you build your form, you have only to use the requested field type, with appropriate choices.

webhdx · 2020-02-24T14:15:38Z

src/lib/Form/Type/Search/SearchType.php

+                'search_language',
+                ConfiguredLanguagesChoiceType::class,
+                [
+                    'required' => false,


See my above comment. Here you could just use choice_loader option on LanguageChoiceType to avoid adding very specific ConfiguredLanguagesChoiceType class.

webhdx · 2020-02-24T14:36:29Z

src/bundle/Controller/SearchController.php

@@ -55,7 +56,7 @@ class SearchController extends Controller

    /**
     * @param \eZ\Publish\API\Repository\SearchService $searchService
-     * @param \EzSystems\EzPlatformAdminUi\Tab\Dashboard\PagerContentToDataMapper $pagerContentToDataMapper
+     * @param \EzSystems\EzPlatformAdminUi\Search\PagerContentToDataMapper $pagerSearchContentToDataMapper


Wrong type, should be \EzSystems\EzPlatformAdminUi\Search\PagerSearchContentToDataMapper

webhdx · 2020-02-24T14:43:27Z

src/lib/Tab/Dashboard/PagerContentToDataMapper.php

+/**
+ * @deprecated This class has been moved to \EzSystems\EzPlatformAdminUi\Search\PagerContentToDataMapper
+ */
+class PagerContentToDataMapper extends \EzSystems\EzPlatformAdminUi\Search\PagerContentToDataMapper


If I'm not mistaken \EzSystems\EzPlatformAdminUi\Search\PagerContentToDataMapper is not used anywhere but on a dashboard tab. If so, please remove \EzSystems\EzPlatformAdminUi\Search\PagerContentToDataMapper and extend AbstractPagerContentToDataMapper here.

This \EzSystems\EzPlatformAdminUi\Search\PagerContentToDataMapper is using in dashboard tabs, but the results becomes from Search engines (at least for now). I'm not sure about move AbstractPagerContentToDataMapper to dashboard "domain" because this class are extending also by \EzSystems\EzPlatformAdminUi\Search\PagerSearchContentToDataMapper, which is not a part of dashboard "domain", but all three classes are connected by search feature now. Maybe we should move them all to namespace like "Mapper" or "PagerContent" for more independent .. what do you think @webhdx ?

src/lib/Tab/Dashboard/PagerContentToDataMapper.php

src/lib/Form/Type/ChoiceList/Loader/ConfiguredLanguagesChoiceLoader.php

…oader.php Co-Authored-By: Adam Wójs <adam.wojs@ez.no>

lserwatka · 2020-02-28T13:25:42Z

Could you merge it up?

EZP-31299 - Added 'search in specific language' feature

30ef9f8

michal-myszka requested a review from adamwojs February 4, 2020 13:07

michal-myszka mentioned this pull request Feb 4, 2020

EZP-31299: Added language mask to LSE search index ezsystems/ezpublish-kernel#2940

Merged

5 tasks

Michał Myszka and others added 6 commits February 6, 2020 09:48

Remove "useAlwaysAvailable" from search form

85aac2e

Added available translations for search results, removed "displayLang…

13b44f0

…uage"

Added not enabled languages for searching, new redirects from search …

95f4fba

…form (to chosen translation)

Added auto submit on language select

8afe4cb

Merge branch '1.5' of https://github.com/ezsystems/ezplatform-admin-ui …

0bebf44

…into ezp-31299-additional-search-languages-filter

Mark template as unused / deprecated

bacf134

michal-myszka requested review from mikadamczyk, ViniTou and andrerom February 14, 2020 09:43

CS Fix

20e873f

adamwojs reviewed Feb 18, 2020

View reviewed changes

alongosz requested changes Feb 18, 2020

View reviewed changes

michal-myszka and others added 4 commits February 18, 2020 13:06

Update src/bundle/Controller/SearchController.php

a8cde15

Co-Authored-By: Andrew Longosz <alongosz@users.noreply.github.com>

After CR changes

a6082a4

After CR changes

c51c6d7

EZP-31396: As an Administrator I want to see the popup with language …

7e6de36

…choose after click the edit button on the search results

dew326 requested review from GrabowskiM and lucasOsti February 20, 2020 11:27

dew326 added 4 commits February 20, 2020 14:31

fixed multiple selectors and position

05cd240

added missing value

513e4ef

added more padding

0c8b033

remove early return

268b409

GrabowskiM reviewed Feb 20, 2020

View reviewed changes

src/bundle/Resources/public/js/scripts/button.translation.edit.js Outdated Show resolved Hide resolved

GrabowskiM self-requested a review February 20, 2020 13:56

GrabowskiM approved these changes Feb 20, 2020

View reviewed changes

lucasOsti approved these changes Feb 20, 2020

View reviewed changes

Michał Myszka added 3 commits February 21, 2020 15:31

Fixup!

6fe7093

New page content data mapper (PagerSearchContentToDataMapper) for sea…

bf75a1c

…rch results and refactor PagerContentToDataMapper

Fix CS

36d8ac8

michal-myszka self-assigned this Feb 24, 2020

adamwojs reviewed Feb 24, 2020

View reviewed changes

alongosz changed the title ~~EZP-31299 - Added 'search in specific language' feature~~ EZP-31299: Added 'search in specific language' feature Feb 24, 2020

alongosz reviewed Feb 24, 2020

View reviewed changes

ViniTou reviewed Feb 24, 2020

View reviewed changes

Michał Myszka added 2 commits February 24, 2020 13:21

BC break removed

f6451a2

After CR

d7210df

alongosz reviewed Feb 24, 2020

View reviewed changes

src/lib/Tab/Dashboard/PagerContentToDataMapper.php Show resolved Hide resolved

webhdx suggested changes Feb 24, 2020

View reviewed changes

Michał Myszka added 2 commits February 25, 2020 15:36

After CR

2d55d31

After CR changes ...

810a5ac

michal-myszka requested review from adamwojs and webhdx February 26, 2020 09:07

webhdx approved these changes Feb 26, 2020

View reviewed changes

adamwojs approved these changes Feb 26, 2020

View reviewed changes

src/lib/Form/Type/ChoiceList/Loader/ConfiguredLanguagesChoiceLoader.php Outdated Show resolved Hide resolved

Update src/lib/Form/Type/ChoiceList/Loader/ConfiguredLanguagesChoiceL…

c2948cb

…oader.php Co-Authored-By: Adam Wójs <adam.wojs@ez.no>

michal-myszka added the Ready for QA label Feb 26, 2020

alongosz approved these changes Feb 26, 2020

View reviewed changes

katarzynazawada approved these changes Feb 28, 2020

View reviewed changes

katarzynazawada added QA approved and removed Ready for QA labels Feb 28, 2020

lserwatka merged commit b6cf000 into 1.5 Feb 28, 2020

lserwatka deleted the ezp-31299-additional-search-languages-filter branch February 28, 2020 13:25

alongosz mentioned this pull request Mar 6, 2020

Merged branch 1.5 into master (EZP-31356, EZP-31395, EZP-31299, EZP-31066) #1269

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EZP-31299: Added 'search in specific language' feature #1223

EZP-31299: Added 'search in specific language' feature #1223

michal-myszka commented Feb 4, 2020 •

edited by alongosz

Loading

adamwojs Feb 18, 2020

adamwojs Feb 18, 2020

adamwojs Feb 18, 2020

alongosz Feb 18, 2020

michal-myszka Feb 18, 2020

alongosz Feb 24, 2020

dew326 commented Feb 20, 2020 •

edited

Loading

adamwojs Feb 24, 2020

adamwojs Feb 24, 2020

adamwojs Feb 24, 2020

adamwojs Feb 24, 2020

adamwojs Feb 24, 2020

adamwojs Feb 24, 2020

alongosz Feb 24, 2020

alongosz Feb 24, 2020

michal-myszka Feb 24, 2020

alongosz Feb 24, 2020

ViniTou Feb 24, 2020 •

edited

Loading

alongosz Feb 24, 2020

ViniTou Feb 24, 2020

michal-myszka Feb 24, 2020

ViniTou Feb 24, 2020

michal-myszka Feb 24, 2020

michal-myszka commented Feb 24, 2020

alongosz commented Feb 24, 2020

webhdx Feb 24, 2020

michal-myszka Feb 25, 2020

webhdx Feb 24, 2020

webhdx Feb 24, 2020

webhdx Feb 24, 2020

michal-myszka Feb 25, 2020

lserwatka commented Feb 28, 2020

	$this->languageChoiceLoader = $languageChoiceLoader;
	parent::__construct($languageChoiceLoader);

	* @param array $contentTypeIds
	* @param int[] $contentTypeIds

	* @param Pagerfanta $pager
	* @param \Pagerfanta\Pagerfanta $pager

EZP-31299: Added 'search in specific language' feature #1223

EZP-31299: Added 'search in specific language' feature #1223

Conversation

michal-myszka commented Feb 4, 2020 • edited by alongosz Loading

Checklist:

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

Choose a reason for hiding this comment

dew326 commented Feb 20, 2020 • edited Loading

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

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

ViniTou Feb 24, 2020 • edited Loading

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

Choose a reason for hiding this comment

michal-myszka commented Feb 24, 2020

alongosz commented Feb 24, 2020

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

Choose a reason for hiding this comment

lserwatka commented Feb 28, 2020

michal-myszka commented Feb 4, 2020 •

edited by alongosz

Loading

dew326 commented Feb 20, 2020 •

edited

Loading

ViniTou Feb 24, 2020 •

edited

Loading