Implemented EZP-21640: Implement RelationList search criterion

[vaxvhbe]

https://jira.ez.no/browse/EZP-21640

Currently, it is not possible to search for Content related (through RelationList) to a specific Content.
In Legacy, this criterion would basically add an additional join to table ezcontentobject_link.

$query = new Query();
$query->criterion = new Criterion\RelationList(
    // Field name
    "relation_field",
    // Operator
    Operator::CONTAINS,
    // Content ID
    42
);
$result = $this->getRepository()->getSearchService()->findContent( $query );

Or:

$query = new Query();
$query->criterion = new Criterion\RelationList(
    // Field name
    "relation_field",
    // Operator
    Operator::IN,
    // Content ID
    array( 42, 43, 44 )
    );
$result = $this->getRepository()->getSearchService()->findContent( $query );

[ezrobot]

This Pull Request does not respect our Coding Standards, please, see the report below:

FILE: ...istence/Legacy/Content/Search/Gateway/CriterionHandler/RelationList.php
--------------------------------------------------------------------------------
FOUND 14 ERROR(S) AFFECTING 7 LINE(S)
--------------------------------------------------------------------------------
 120 | ERROR | A space is required after opening parenthesis of function call
 120 | ERROR | A space is required before closing parenthesis of function call
 124 | ERROR | A space is required after opening parenthesis of function call
 124 | ERROR | A space is required before closing parenthesis of function call
 125 | ERROR | A space is required after opening parenthesis of function call
 125 | ERROR | A space is required before closing parenthesis of function call
 128 | ERROR | A space is required after opening parenthesis of function call
 128 | ERROR | A space is required before closing parenthesis of function call
 129 | ERROR | A space is required after opening parenthesis of function call
 129 | ERROR | A space is required before closing parenthesis of function call
 132 | ERROR | A space is required after opening parenthesis of function call
 132 | ERROR | A space is required before closing parenthesis of function call
 133 | ERROR | A space is required after opening parenthesis of function call
 133 | ERROR | A space is required before closing parenthesis of function call
--------------------------------------------------------------------------------


FILE: .../eZ/Publish/Core/Persistence/Legacy/Tests/Content/SearchHandlerTest.php
--------------------------------------------------------------------------------
FOUND 6 ERROR(S) AFFECTING 3 LINE(S)
--------------------------------------------------------------------------------
 1623 | ERROR | Empty array declaration must have no space between the
      |       | parentheses
 1623 | ERROR | Without arguments, space are forbidden in function call
 1667 | ERROR | Empty array declaration must have no space between the
      |       | parentheses
 1667 | ERROR | Without arguments, space are forbidden in function call
 1711 | ERROR | Empty array declaration must have no space between the
      |       | parentheses
 1711 | ERROR | Without arguments, space are forbidden in function call
--------------------------------------------------------------------------------

[andrerom]

This is awesome! :)
With the CS issues fixed and a squash: +1 from me, review ping @pspanja

[pspanja]

Specification type should actually be Specifications::TYPE_INTEGER | Specifications::TYPE_STRING, as in future NoSQL storage id will be a hash (for this reason we always hint ids as mixed and not int).

[andrerom]

Side note: @pspanja / @patrickallaert is the failure in commit above caused by postgres lack of ordering? https://travis-ci.org/ezsystems/ezpublish-kernel/jobs/12238878

[patrickallaert]

@andrerom

Side note: @pspanja / @patrickallaert is the failure in commit above caused by postgres lack of ordering? https://travis-ci.org/ezsystems/ezpublish-kernel/jobs/12238878

Yes, well, in fact it is MySQL that returns values too frequently ordered by primary key even if no sorting asked ;-) But I saw some changes in this regard with MariaDB 5.5 (MySQL 5.5 as well?) where returned values were more randomly ordered.

All tests needs indeed to be tested without any fixed ordering.

[patrickallaert]

Something @taenadil didn't mention is that...

$query->criterion = new Criterion\RelationList(
    "relation_field",
    Operator::EQ,
    42
);

looks for content where the Content ID 42 resides in the RelationList field "relation_field",...

$query->criterion = new Criterion\RelationList(
    "relation_field",
    Operator::EQ,
    array( 42, 43 )
);

looks for content where both the Content ID 42 AND 43 resides in the RelationList field "relation_field", and that...

$query->criterion = new Criterion\RelationList(
    "relation_field",
    Operator::IN,
    array( 42, 43 )
);

looks for content where one of Content ID 42 OR 43 resides in the RelationList field "relation_field".

The following is not supported, and I am wondering if it should:

$query->criterion = new Criterion\RelationList(
    "relation_field",
    Operator::IN,
    array( array( 42, 43 ), array( 2, 3 ) )
);

This would theoretically search for content where in the RelationList field we would have both (42 AND 43) OR (2 AND 3).

Other than that, huge +1 from me :-)

Congrats @taenadil ! (#kernel-contribution-of-the-year ?)

[andrerom]

@patrickallaert Nice, but one possible thing: @pspanja introduced a ::CONTAINS word recently, so maybe that one is more suited to deal with the OR condition?

[patrickallaert]

@andrerom

@patrickallaert Nice, but one possible thing: @pspanja introduced a ::CONTAINS word recently, so maybe that one is more suited to deal with the OR condition?

Might be, but we miss a clear example about how to use EQ vs IN vs CONTAINS. It could also make sense to have arrays in all cases, right? @pspanja might elaborate on this.

[pspanja]

@patrickallaert, @andrerom, @taenadil

I'm ok how IN is now implemented. Regarding:

This would theoretically search for content where in the RelationList field we would have both (42 AND 43) OR (2 AND 3).

I would rather not go for this, as it is already possible to achieve the same by using LogicalOr criterion and EQ operator.

For EQ behavior is I think not completely intuitive. Here I would expect that relation list contains all and only all given values, meaning no other values that are not given exist in relation list.

On the other hand, CONTAINS should work as EQ works now, meaning it should accept both single value and array of values, checking if the relation list contains all given values and allowing other values that we do not care about.

I think this is intuitive and would cover all bases, but would probably be a pain to implement, or even not possible to do in a sane way.

So this might be the best we can get, but I would suggest renaming EQ to CONTAINS.

What do you think?

[patrickallaert]

@pspanja

What do you think?

Perfectly agreed with all of your remarks.

@taenadil +1 given you rename EQ to CONTAINS.

[vaxvhbe]

Commits squashed in one commit and EQ renamed to CONTAINS

Review ping: @pspanja @andrerom

[pspanja]

+1 as soon as Travis agrees :)

Great work @taenadil!

[lolautruche]

Travis seems to agree. Can we merge it now @pspanja @patrickallaert @andrerom ?

[patrickallaert]

Travis seems to agree. Can we merge it now @pspanja @patrickallaert @andrerom ?

Seems so, yes 👍

[pspanja]

Merging... @andrerom 5.2 right? :)

[lolautruche]

@pspanja I guess so

[andrerom]

I'm all +1 but I was about to ask if it should be renamed to just "Relation", cause it works across all relation types right? There is nothing in here specific to RelationList FieldType? Or is there other reasons to call it "List" maybe ?

[pspanja]

@andrerom True, but we still might want to implement criterion for Content-level relations. So maybe "FieldRelation"?

Edit: Actually, it might make sense to adapt this for all Relation types.

[andrerom]

Well, one FieldRelation and one ContentRelation with code reuse between each other can be good approach, otherwise field identifier needs to be made optional and we need to add relation type as param as well.

[pspanja]

@andrerom agreed, I'll open a PR.