Fixed EZP-31630: error when running location query

[bdunogier]

Question	Answer
JIRA issue	EZP-31630
Bug/Improvement	yes
New feature	kind of
Target version	1.x and 2.x
BC breaks	no
Tests pass	yes
Doc needed	yes

When a QueryType returns a LocationQuery that uses Location specific criteria, execution will fail since findContent is always used.

Resolved by testing the type of the returned Query object, and executing with the applicable SearchService method. The results are still Content items, not locations.

TODO

• Update tests if applicable
• Merge to master

[bdunogier]

@michal-myszka any idea why Travis won't run ?

[adamwojs]

@michal-myszka any idea why Travis won't run ?

@mnocon is the right person here 😉

[bdunogier]

Oops, thank you @adamwojs :)
So you don't know either ?

P.S. does travis run when you change a PR from draft to non-draft ?

[mnocon]

Not sure what happened here, here's what I can tell:
this PR was against master at the beginning and it failed to build with message:

 GitHub payload is missing a merge commit (mergeable_state: "draft", merged: false) 

As far as I know Travis usually builds draft PRs, but maybe there were conflicts between this PR and master? This would stop the build from triggering.

Changing the base branch (from master to 1.0) does not trigger a build, so no change here.

Can you try to trigger the build again, so that the base change is taken into account? Then we will see what happens (you have to do it by git, something like git commit --amend && git push --force-with-lease should be enough)

[bdunogier]

Oh, yes, I also rebased the branch against 1.0 and pushed it, I guess that's when it didn't run. Thank you !

[bdunogier]

Okay, we are green now, ready for QA.

[bdunogier]

We have an issue here: if an item has several locations that match the criteria, the result set will contain the same content item several times. I'm considering adding a filter during the iteration to prevent that, and only return the first occurrence of each content item. But the second problem is pagination...

What do you think, @adamwojs ? :)

[bdunogier]

So you're approving despite the issue, @adamwojs ? :)

[adamwojs]

So you're approving despite the issue, @adamwojs ? :) 

Sorry, I missed somehow your comment. I see the following options here

1. Document that content query cannot be used with location specific criteria / sort clauses
2. Check if query contains criterion / sort clause which is location specific and call findContent/findLocation base on result.  
3. Modify LocationQuery in few ways:
   1. Remove location specific criteria / sort clauses 
   2. Append IsMainLocation criterion to LocationQuery filter 

or introduce location-query field type

[bdunogier]

1. Document that content query cannot be used with location specific criteria / sort clauses
   I think that it is a bit brutal, especially since we can't warn about that during content type edition. The query type registry doesn't know if a QueryType is a Location or Content one.

2. Check if query contains criterion / sort clause which is location specific and call findContent/findLocation base on result.

This is what this PR does, with the consequences explained above.

3. Modify LocationQuery in few ways:
   Remove location specific criteria / sort clauses
   Append IsMainLocation criterion to LocationQuery filter

I think it won't be satisfactory for users, as the results will be very unpredictable. For isMainLocation, for instance, it could very well be that the tree you're querying only contains alternative locations, not main ones, and we can't really know about that.

I need to think about it some more... but it feels likely that we will need to evolve queries and query types in general to do this right.

[bdunogier]

Append IsMainLocation criterion to LocationQuery filter

This might be the most agile option out of all of them. It doesn't make everything possible, but it fixes it for main locations, at least.

[bdunogier]

One more kinda related thing about content / location queries: I think that we should push the concept of location to a "secondary" feature, so that content items, their type and fields are the first citizen of our API. Do you see the idea, @adamwojs ?

[lserwatka]

@bdunogier we need a rebase here