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-30996: RelationField return no results if field searchable in more than 1 content type #154

Open
wants to merge 1 commit into
base: 1.7
Choose a base branch
from

Conversation

ITernovtsii
Copy link

@ITernovtsii ITernovtsii commented Oct 1, 2019

PR:

  • Fix Criterion\FieldRelation visitor for case when 2+ content types have same field searchable
  • Add tests

Issue

If you have 2 content types (type1, type2) with the same attribute identifier (attr1), and run a search for attr1 Criterion\Operator::CONTAINS array("some_value"), then in solr search the query ends up looking like type1_attr1="some_value" AND type2_attr1="some_value" so nothing gets returned.
(In legacy search, it will work as expected)

Open question

I am not sure about this part, which I left from original implementation:

$preparedValues = (array)$this->mapSearchFieldvalue($value, $fieldType);
foreach ($preparedValues as $prepValue) {

In the core, there is no way to get multiple values in $preparedValues.
mapSearchFieldvalue always return string or int type.

The only use-case I can think about - some platform user extended it and use "int|string" identifier as criteria value but have that single identifier mapped to multiple values inside mapSearchFieldvalue
While this use-case looks at least weird, and such implementation potentially can rely not on "AND" logic rule, I would propose to remove this part in this PR.
Please confirm it's okay and I'll update this PR to not include it.

@ITernovtsii ITernovtsii changed the base branch from 2.0 to 1.5 December 20, 2019 13:34
@ITernovtsii ITernovtsii force-pushed the 2.0-fix-field-relation branch 2 times, most recently from fa3b0da to d345dd1 Compare December 23, 2019 12:35
@ITernovtsii
Copy link
Author

@andrerom as discussed, rebased to 1.5 (and fixed compatibility with php 5.6)
Please review and let me know if this can be removed:

$preparedValues = (array)$this->mapSearchFieldvalue($value, $fieldType);
foreach ($preparedValues as $prepValue) {

Thanks.

@andrerom
Copy link
Contributor

andrerom commented Jan 2, 2020

let me know if this can be removed:

hmm, why do you think this should be removed? Or did we already discussed that somewhere?

@ITernovtsii
Copy link
Author

ITernovtsii commented Jan 13, 2020

let me know if this can be removed:

hmm, why do you think this should be removed? Or did we already discussed that somewhere?

We didn't discuss it yet, it was only mentioned in the description:

In the core, there is no way to get multiple values in $preparedValues.
mapSearchFieldvalue always return string or int type.

@andrerom
Copy link
Contributor

@adamwojs / @alongosz POV?

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.

I'm not really familiar with this Criterion, so I need complete steps to reproduce: the entire search query, the minimal setup (what Content Types and content items were created?) and the expected result. Both the issue description and tests do not provide full information, only pieces.

lib/Query/Common/CriterionVisitor/Field/FieldRelation.php Outdated Show resolved Hide resolved
@ITernovtsii ITernovtsii force-pushed the 2.0-fix-field-relation branch 4 times, most recently from 60a8591 to ce27e3f Compare January 15, 2020 11:20
@ITernovtsii
Copy link
Author

@alongosz
To reproduce it on clean install.
Add "ezobjectrelationlist" field to Article and Folder content types.
Make sure fields are searchable, and they have same identifier.
Let's add rel_folder to both.

Create one folder object, and select root folder in "rel_folder" field.
Try to find it with search service:

$rootContentId = 1;
$found = $this->searchService->findContent(new Query([
    'filter' => new Criterion\FieldRelation('rel_folder', Criterion\Operator::CONTAINS, [$rootContentId])
]))->totalCount;
var_dump($found);

It will work with legacy search, and not on solr.

I tested it by editing a vendor/ezsystems/ezplatform-admin-ui/src/bundle/Controller/SearchController.php - searchAction locally and visiting localhost/admin/search page.
To see that it works as described in PR description, you can go back to "Article" content type, and mark "rel_folder" as not searchable. After that, you'll see that object found properly.

@andrerom andrerom changed the title EZP-30996 - fix SOLR FieldRelation criteria visitor EZP-30996: RelationField return no results if field searchable in more than 1 content type Jan 31, 2020
@ITernovtsii ITernovtsii changed the base branch from 1.5 to 1.7 December 16, 2020 06:42
@ITernovtsii ITernovtsii changed the base branch from 1.7 to 1.5 December 16, 2020 07:22
@ITernovtsii ITernovtsii changed the base branch from 1.5 to 1.7 December 16, 2020 07:33
@ITernovtsii
Copy link
Author

@alongosz Rebased to 1.7, any updates on this one?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants