EZP-26780 Implement DateMetadata criterion parser in REST #1861

kamilmusial · 2016-12-09T14:14:54Z

JIRA: https://jira.ez.no/browse/EZP-26780

Currently there's no OperatorCriterion, so there's no easy way to set $operator for DateMetadataCriterion like it's done in other parsers.

That could be done by simply passing a string value to criterion constructor, but that'd omit a specification whitelist. So there's quick check in an array of abstract class reflection constants to validate an operator input before return a criterion object.

Another approach could be to implement an OperatorCriterion parser, but that should be another issue(?)

kamilmusial · 2016-12-09T14:15:03Z

ping @andrerom @bdunogier

andrerom · 2016-12-11T18:39:08Z

eZ/Publish/Core/REST/Server/Input/Parser/Criterion/DateMetadata.php

+        }
+        $target = $data['DateMetadataCriterion']['Target'];
+
+        $reflector = new ReflectionClass('eZ\Publish\API\Repository\Values\Content\Query\Criterion\Operator');


would be good to not have to use reflection

kamilmusial · 2016-12-12T11:16:06Z

ping @andrerom
I created simple mapping helping to check for available operators. It's hard to fetch the constants from an abstract class without reflection. Maybe you have some suggestion about that solution?

andrerom · 2016-12-12T14:35:52Z

eZ/Publish/API/Repository/Values/Content/Query/Criterion/Operator.php

+    /**
+     * @return array
+     */
+    public static function getAvaliableOperators()


afaik this does not have any meaning in the PHP API, as operators are never referred to by string names like "EQ" and so on.

Documentation referes to those names:
https://doc.ez.no/display/EZP/Criteria+reference

It does not say to use "=", ">=", etc.

As far as I read that doc it refers to the constants, but it is indeed hard to tell on this page as it does not have any examples.

And this is what I referred to above, you don't use the API by passing in string values EQ and so on, or = for that matter, you use constants.

andrerom · 2016-12-12T14:38:23Z

eZ/Publish/Core/REST/Server/Input/Parser/Criterion/DateMetadata.php

+
+        if (!isset($data['DateMetadataCriterion']['Operator'])) {
+            $data['DateMetadataCriterion']['Operator'] = 'EQ';
+        } elseif (!in_array($data['DateMetadataCriterion']['Operator'], array_keys(Operator::getAvaliableOperators()))) {


You can move the method to this class as private function getAvaliableDateMetadateRESTOperators(), and it can return plain array with the available operators.

kamilmusial · 2016-12-13T07:25:02Z

@andrerom Changed.

andrerom · 2016-12-13T18:39:14Z

eZ/Publish/Core/REST/Server/Input/Parser/Criterion/DateMetadata.php


 /**
 * Parser for ViewInput Criterion.
 */
 class DateMetadata extends BaseParser
 {
+    /** @var array */
+    public $availableOperators = [


I was thinking about changing this value or inheritance, but after rethink it can be changed I guess

andrerom · 2016-12-13T18:41:43Z

@bdunogier looks ok to me now besides comment, but unsure about the whole OperatorCriterion as I'm not aware how this is done in other criteria.

kamilmusial · 2016-12-14T07:29:34Z

@andrerom AFAIK there're no operators in other criteria or they are not implemented.

andrerom · 2016-12-16T16:56:50Z

@andrerom AFAIK there're no operators in other criteria or they are not implemented.

ok, somewhat clear.

note to self/other-reviewers then: check spec and try to get how operators were intended to work, now that PR changed values from string versions of constants (EQ, LIKE, ..), to the actual values which afaik are supposed to be internal, hence the use of constants.

pspanja · 2016-12-19T08:08:52Z

@andrerom @kamilmusial
The names of the constants in the Operator class should be used. They could be mapped to the actual value through a simple case switch, for example:

switch ($data['DateMetadataCriterion']['Operator']) {
    case 'IN':
        $operator = Operator::IN;
        break:
    case 'GTE':
        $operator = Operator::GTE;
        break:
    case 'BETWEEN':
        $operator = Operator::BETWEEN;
        break:
    ...
}

bdunogier

The code looks okay.

After thinking about it for quite a bit, I'm wondering if we should move this up one level... we could conceal the whole operator debate by abstracting the criterion like this:

"ModifiedSince": <timestamp>
"PublishedAfter": <timestamp>
"ModifiedBetween": [<timestamp1>, <timestamp2>]

It would be much easier to use for REST consumers, and much more semantic/readable.

It is a bit more work for us to parse, but if we make this logic part of the parser dispatcher, it shouldn't be that complicated. Instead of a 1:1 match of the criterion's name, we could first detect this "operator suffix", and pass it to the parser when it gets invoked ? That way, we can keep the structs simple, and it also works for any criterion that needs an operator.

The main drawback that I can see is the non handling of the "than or equal" (e.g. GT vs GTE). But I'm kind of thinking that this case isn't that frequent, and it can always be formulated in other ways.

Please let me know what you think.

bdunogier · 2017-01-03T10:43:56Z

eZ/Publish/Core/REST/Server/Input/Parser/Criterion/DateMetadata.php

+        $target = $data['DateMetadataCriterion']['Target'];
+
+        if (!isset($data['DateMetadataCriterion']['Operator'])) {
+            $data['DateMetadataCriterion']['Operator'] = '=';


You could skip this completely, and set a $operator to null. The Criterion's constructor will set it either to EQ or IN depending on the value.

bdunogier · 2017-01-03T10:45:20Z

eZ/Publish/Core/REST/Server/Input/Parser/Criterion/DateMetadata.php

+        }
+        $value = $data['DateMetadataCriterion']['Value'];
+
+        return new DateMetadataCriterion($target, $data['DateMetadataCriterion']['Operator'], $value);


Small consistency remark: why set a local variable for Value and not for Operator ? I'd probably be consistent here, and also set a $operator after the checks above.

bdunogier · 2017-01-03T10:45:56Z

eZ/Publish/Core/REST/Server/Tests/Input/Parser/Criterion/DateMetadataTest.php

+                ['DateMetadataCriterion' => ['Target' => 'created', 'Operator' => '=', 'Value' => 1033920830]],
+                new DateMetadataCriterion('created', '=', 1033920830),
+            ],
+        ];


Could you add a test case where the operator isn't set ?

bdunogier · 2017-01-03T10:46:47Z

eZ/Publish/Core/REST/Server/Tests/Input/Parser/Criterion/DateMetadataTest.php

+     * @expectedException \eZ\Publish\Core\REST\Common\Exceptions\Parser
+     * @expectedExceptionMessage Invalid <DateMetadataCriterion> format
+     */
+    public function testParseExceptionOnInvalidCriterionFormat()


Given that you have multiple error tests, you could use a dataProvider for them as well, since you use one for valid cases.

alongosz · 2019-06-19T17:10:46Z

REST implementation has been moved to eZ Platform REST Bundle. We'll have to implement it there. Closing this one.

kamilmusial force-pushed the master branch from f10466e to a823c5d Compare December 9, 2016 14:20

andrerom reviewed Dec 11, 2016

View reviewed changes

kamilmusial force-pushed the master branch from a36c79d to 2b8de0d Compare December 12, 2016 11:17

andrerom suggested changes Dec 12, 2016

View reviewed changes

kamilmusial force-pushed the master branch 2 times, most recently from 3c13554 to d5aaf4e Compare December 13, 2016 07:21

kamilmusial force-pushed the master branch 3 times, most recently from 08bde8d to f049fb4 Compare December 13, 2016 07:26

andrerom reviewed Dec 13, 2016

View reviewed changes

EZP-26780 Implement DateMetadata criterion parser in REST

0614e5d

kamilmusial closed this Dec 14, 2016

kamilmusial force-pushed the master branch from f049fb4 to 34f8a09 Compare December 14, 2016 07:38

kamilmusial reopened this Dec 14, 2016

bdunogier reviewed Jan 3, 2017

View reviewed changes

alongosz closed this Jun 19, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EZP-26780 Implement DateMetadata criterion parser in REST #1861

EZP-26780 Implement DateMetadata criterion parser in REST #1861

kamilmusial commented Dec 9, 2016

kamilmusial commented Dec 9, 2016

andrerom Dec 11, 2016

kamilmusial commented Dec 12, 2016

andrerom Dec 12, 2016

lserwatka Dec 12, 2016

andrerom Dec 12, 2016 •

edited

Loading

andrerom Dec 12, 2016 •

edited

Loading

andrerom Dec 12, 2016

kamilmusial commented Dec 13, 2016

andrerom Dec 13, 2016

kamilmusial Dec 14, 2016

andrerom commented Dec 13, 2016 •

edited

Loading

kamilmusial commented Dec 14, 2016

andrerom commented Dec 16, 2016 •

edited

Loading

pspanja commented Dec 19, 2016

bdunogier left a comment

bdunogier Jan 3, 2017

bdunogier Jan 3, 2017

bdunogier Jan 3, 2017

bdunogier Jan 3, 2017

alongosz commented Jun 19, 2019

EZP-26780 Implement DateMetadata criterion parser in REST #1861

EZP-26780 Implement DateMetadata criterion parser in REST #1861

Conversation

kamilmusial commented Dec 9, 2016

kamilmusial commented Dec 9, 2016

Choose a reason for hiding this comment

kamilmusial commented Dec 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrerom Dec 12, 2016 • edited Loading

Choose a reason for hiding this comment

andrerom Dec 12, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kamilmusial commented Dec 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrerom commented Dec 13, 2016 • edited Loading

kamilmusial commented Dec 14, 2016

andrerom commented Dec 16, 2016 • edited Loading

pspanja commented Dec 19, 2016

bdunogier left a 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

alongosz commented Jun 19, 2019

andrerom Dec 12, 2016 •

edited

Loading

andrerom Dec 12, 2016 •

edited

Loading

andrerom commented Dec 13, 2016 •

edited

Loading

andrerom commented Dec 16, 2016 •

edited

Loading