Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

EZP-31299: Added language mask to LSE search index #2940

Merged
merged 20 commits into from
Feb 28, 2020

Conversation

michal-myszka
Copy link
Contributor

@michal-myszka michal-myszka commented Feb 4, 2020

Question Answer
JIRA issue EZP-31299
Bug/Improvement yes
New feature yes
Target version 7.5
BC breaks no
Tests pass yes
Doc needed no

This PR's changes improvement searching in languages (in translations).

  • Updated indexing process for legacy SQL search (added language mask to words links)
  • Updated SQL search engine for searching in translations from provided languages list
  • Provided search language filter into ContentSearchHitAdapter
  • Added SQL migration for ezsearch_object_word_link table

Related changes in
ezsystems/ezplatform-admin-ui#1223
ezsystems/ezplatform-solr-search-engine#168

TODO:

@michal-myszka michal-myszka changed the title EZP-31299 - Provided search language filter into ContentSearchHitAdapter EZP-31299 - Search in languages Feb 17, 2020
data/mysql/schema.sql Show resolved Hide resolved
@alongosz alongosz changed the title EZP-31299 - Search in languages EZP-31299: Added language mask to LSE search index Feb 17, 2020
Copy link
Member

@alongosz alongosz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@michal-myszka I've missed one more thing (seems the scripts for the previous release miss the same thing, but out of scope here):

@alongosz alongosz requested a review from adamwojs February 18, 2020 08:55
eZ/Publish/API/Repository/Tests/SearchServiceTest.php Outdated Show resolved Hide resolved
usort(
$searchHits,
function ($a, $b) {
return ($a->valueObject->id < $b->valueObject->id) ? -1 : 1;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use the spaceship operator here. This opportunity may not happen again 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Used (hurray!) :)

eZ/Publish/API/Repository/Tests/SearchServiceTest.php Outdated Show resolved Hide resolved
@@ -136,6 +143,8 @@ public function index(FullTextData $fullTextData)
'ContentClassAttributeID' => $fullTextValue->fieldDefinitionId,
'identifier' => $fullTextValue->fieldDefinitionIdentifier,
'integer_value' => $integerValue,
'language_code' => $fullTextValue->languageCode,
'is_main_and_always_available' => $fullTextValue->isMainAndAlwaysAvailable,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The language_code / is_main_and_always_available values needs to added to

$indexArray[] = ['Word' => $additionalUrlWord,
'ContentClassAttributeID' => $fullTextValue->fieldDefinitionId,
'identifier' => $fullTextValue->fieldDefinitionIdentifier,
'integer_value' => $integerValue, ];
as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, added

bool $useAlwaysAvailable
): SearchResult {
if (false === in_array($findMethod, self::AVAILABLE_FIND_METHODS, true)) {
throw new \InvalidArgumentException(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: missing import and @throws docblock

@@ -17,12 +17,13 @@ class ContentSearchAdapterTest extends ContentSearchHitAdapterTest
/**
* @param Query $query
* @param SearchService $searchService
* @param array
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @param array
* @param array $languageFilter

@@ -37,9 +37,9 @@ protected function setUp()
*
* @return ContentSearchHitAdapter
*/
protected function getAdapter(Query $query, SearchService $searchService)
protected function getAdapter(Query $query, SearchService $searchService, array $languageFilter = [])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: missing @param for $languageFilter

@@ -65,7 +77,7 @@ public function getSlice($offset, $length)
$query->limit = $length;
$query->performCount = false;

$searchResult = $this->searchService->findContent($query);
$searchResult = $this->searchService->findContent($query, $this->languageFilter);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct me if I'm wrong, but it seems that we are missing an assertion ensuring that language Filter is passed to findContent ContentSearchHitAdapterTest.


$setupFactory = $this->getSetupFactory();

if ($setupFactory instanceof LegacySolrSetupFactory) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, this block would look better in a separate data provider.

Copy link
Member

@adamwojs adamwojs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's fine from my POV 😉

@katarzynazawada
Copy link

@lserwatka lserwatka merged commit 557bdfe into 7.5 Feb 28, 2020
@lserwatka lserwatka deleted the ezp-31299-additional-search-languages-filter branch February 28, 2020 13:24
@lserwatka
Copy link
Member

Could you merge it up?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

6 participants