Fix-EZP-27373: Error when edit empty not required date #1996

kmadejski · 2017-05-26T13:30:48Z

JIRA issue: https://jira.ez.no/browse/EZP-27373

Description

If Date FieldType is used and Searchable setting is checked and also default value is set to Empty then it is impossible to save content without setting the value of this field.
API returns an error as described in issue.

alongosz · 2017-05-26T13:54:51Z

eZ/Publish/Core/FieldType/Date/SearchField.php

@@ -29,12 +29,17 @@ class SearchField implements Indexable
     */
    public function getIndexData(Field $field, FieldDefinition $fieldDefinition)
    {
-        $dateTime = new DateTime("@{$field->value->data['timestamp']}");
+        if ($field->value->data['timestamp']) {


Timestamp equal to 0 is still a valid one (1 Jan 1970), though this code would not set it.
I would go for if (isset($field->value->data['timestamp']) && $field->value->data['timestamp'] !== ''), because:

AFAICS when not set, it is null there, so isset solves it.

timestamp array key might not be set at all (probably doesn't happen though).

Unfortunately the string '1495808222' is also a valid timestamp, but '' is not.

However I've been wrong in such cases, so pinging @andrerom and @Nattfarinn :)

@alongosz You are definitely right. Thank you. I changed it as you proposed.

alongosz · 2017-05-26T13:58:13Z

eZ/Publish/Core/FieldType/Date/SearchField.php

+            $dateTime = new DateTime("@{$field->value->data['timestamp']}");
+            $dataValue = $dateTime->format('Y-m-d\\Z');
+        } else {
+            $dataValue = null;


Variable named $dataValue looks weird. Rather $searchFieldValue or just $value.

alongosz · 2017-05-29T08:59:10Z

@kmadejski I took a closer look at PRs and it looks like this is related to #1967 and #1992 as a follow-up on #1942.
This seems to affect DateAndTime field, so same fix for SearchField there is required.

@gggeek You've mentioned in #1942 that you'd prefer here cast to int.
Casting null to int gives us 0, so it means 1 Jan 1970. However if null gets passed here, it rather means that SearchField value should be null, thus not indexed (IMHO).
Is this ok with you?

andrerom · 2017-05-29T09:15:40Z

@kmadejski See the other PR's above, there are some more cases to cover.

kmadejski · 2017-05-29T09:21:05Z

@alongosz @andrerom thank you, I didn't find it before, I will investigate it more.

alongosz · 2017-05-29T09:26:43Z

Note: one of the cases involves Value::fromTimestamp method. I wouldn't be so sure about it, because this method is designed to get int and only int. If it gets something else, like null it should throw InvalidArgument exception, which it already does.

kmadejski · 2017-05-29T10:09:09Z

@alongosz, @andrerom Correct me if I'm wrong, but it seems Value::fromTimestamp is invoked always with int values. See:
https://github.com/ezsystems/ezpublish-kernel/blob/master/eZ/Publish/Core/FieldType/Date/Type.php#L91-L93
and as you mentioned before, if we cast null to int we will get 0, so I think that this code should work well:
https://github.com/ezsystems/ezpublish-kernel/blob/master/eZ/Publish/Core/FieldType/Date/Type.php#L153

The same situation in case of DateAndTime FieldType, see:
https://github.com/ezsystems/ezpublish-kernel/blob/master/eZ/Publish/Core/FieldType/DateAndTime/Type.php#L101-L103
https://github.com/ezsystems/ezpublish-kernel/blob/master/eZ/Publish/Core/FieldType/DateAndTime/Type.php#L170

andrerom · 2017-05-29T11:07:05Z

@kmadejski you are right, and it's documented to only support int.

kmadejski · 2017-05-29T11:10:30Z

@andrerom Thanks, I will prepare fix also for DateAndTime/SearchField.php, it seems to be the same issue there.

andrerom · 2017-05-29T11:12:42Z

@kmadejski Ok, then also check Time, and make sure the originally issue fixed in #1942 is solved for all of them.

kmadejski · 2017-05-29T12:05:23Z

@andrerom fixed also for Time and DateAndTime FieldTypes. I did manual tests and solution looks working as expected.

New code

kmadejski · 2017-05-29T14:01:02Z

@alongosz I changed validation in Time regarding your advice. Thank you.

alongosz

Since Friday I had a feeling I've missed something important while reviewing this PR...
First of all, I wondered why our integration tests don't uncover this case. Well, it turns out we actually test content create with empty values, but not content publish, which triggers getIndexData.

So here's the test to be added to eZ\Publish\API\Repository\Tests\FieldType\BaseIntegrationTest:

    /**
     * Test that publishing (and thus indexing) content with an empty field value does not fail.
     *
     * @depends testCreateContentWithEmptyFieldValue
     *
     * @param \eZ\Publish\API\Repository\Values\Content\Content $contentDraft
     */
    public function testPublishContentWithEmptyFieldValue(Content $contentDraft)
    {
        $this->getRepository(false)->getContentService()->publishVersion(
            $contentDraft->versionInfo
        );
    }

Second, I looked into how exactly Field object is initialized and my change requests add unnecessary complexity, so they're wrong. Below there are correct simplifications with explanation as to why.
(@kmadejski please don't kill me... :-))

alongosz · 2017-05-30T09:56:32Z

eZ/Publish/Core/FieldType/Date/SearchField.php

@@ -29,12 +29,17 @@ class SearchField implements Indexable
     */
    public function getIndexData(Field $field, FieldDefinition $fieldDefinition)
    {
-        $dateTime = new DateTime("@{$field->value->data['timestamp']}");
+        if (isset($field->value->data['timestamp']) && $field->value->data['timestamp'] !== '') {


if ($field->value->data !== null) is enough because:

DateConverter ensures that the data property remains null or is an array with timestamp key containing actual integer.

Content Mapper properly casts value set to dataInt retriveved from the db.

alongosz · 2017-05-30T10:02:23Z

eZ/Publish/Core/FieldType/DateAndTime/SearchField.php

@@ -28,10 +29,17 @@ class SearchField implements Indexable
     */
    public function getIndexData(Field $field, FieldDefinition $fieldDefinition)
    {
+        if (isset($field->value->data['timestamp']) && $field->value->data['timestamp'] !== '') {


if ($field->value->data !== null) is enough because DateAndTime Converter ensures the same thing as above.

And we are sure this only comes from storage and not user (via input validation and such)?

And we are sure this only comes from storage and not user (via input validation and such)?

Yes. getIndexData is called by eZ\Publish\SPI\Search\Handler::indexContent which always gets its content from eZ\Publish\SPI\Persistence\Content\Handler::load

alongosz · 2017-05-30T10:06:11Z

eZ/Publish/Core/FieldType/Time/SearchField.php

@@ -28,10 +29,17 @@ class SearchField implements Indexable
     */
    public function getIndexData(Field $field, FieldDefinition $fieldDefinition)
    {
+        if ($field->value->data !== null && $field->value->data !== '') {


if ($field->value->data !== null) is enough because TimeConverter also ensures data remains null if not set.

alongosz · 2017-05-30T10:31:31Z

I've also found another bug related to this line.
Basically if you set your DateTime to 1st Jan 1970, which is 0, reloading content will not set data property. Removing || $value->dataInt == 0 seems to fix it.

Not sure if this can be included here or should be a separate PR. @andrerom?

Test cases for eZ\Publish\API\Repository\Tests\FieldType\DateIntegrationTest:

    /**
     * @return \eZ\Publish\API\Repository\Values\Content\VersionInfo
     */
    public function testCreateContentWithZeroDate()
    {
        $fieldValue = DateValue::fromTimestamp(0);
        $content = $this->createContent($fieldValue);
        $loadedContent = $this->getRepository(false)->getContentService()->loadContentByContentInfo($content->contentInfo);
        self::assertEquals($content, $loadedContent);

        return $loadedContent->versionInfo;
    }

    /**
     * @depends testPublishContentWithZeroDate
     *
     * @param \eZ\Publish\API\Repository\Values\Content\VersionInfo $versionInfo
     */
    public function testPublishContentWithZeroDate(VersionInfo $versionInfo)
    {
        $this->getRepository(false)->getContentService()->publishVersion($versionInfo);
    }

kmadejski · 2017-05-30T10:32:18Z

@alongosz don't worry, your advice are always very helpful, thank you! I'll fix it in few minutes.

andrerom · 2017-05-30T10:33:33Z

Not sure if this can be included here or should be a separate PR. @andrerom?

lets make that separate, this is already covering to many things (regression and second look at the original issue).

Keep that small and we can fast track it and rebase this on top, if you require it you can temporarily set base branch to the new PR here.

alongosz · 2017-05-30T13:20:37Z

And we've uncovered two separate issues...

InputParserPassTest::testProcess fails for Symfony 3.3, because service ids are case sensitive (or rather will be in the future). To fix this in a BC way let's change UnitTest to lowercase in this method.
Empty MapLocation for Elasticsearch is incorrectly mapped, so fails (hurrah, we've found actual bug!)
Looking at ES logs it seems that the method
eZ\Publish\Core\Search\Elasticsearch\Content\FieldValueMapper\GeoLocationMapper::map should have an extra check, like:

        if (!isset($field->value['latitude'], $field->value['longitude'])) {
            return null;
        }

These probably need to be separate PRs.

andrerom · 2017-05-30T13:32:15Z

Looks good now, but one control question on what we ended up with here:

Same solution was provided also in Time and DateAndTime FieldTypes.

Was Time and DateAndTime actually affected by the timezone issue solved in #1942? Or could they just have been kept as they where (as in just passing on the timezone, if there is one. aka only fix would be to make sure to check that data is set in DateAndTime).

kmadejski · 2017-05-30T13:50:48Z

@andrerom DateAndTime wasn't affected with #1942 because there always was @ in Value::fromTimestamp, but @ was missing in SearchField::getIndexData so it can cause a similar error like this one related to this PR or returns the wrong date (without timezone) to the search index.

In Time problem described in #1942 does not exist because timestamp is passed in method DateTime::setTimestamp which is timezone aware.
There is also @ missing in SearchField::getIndexData, but in this case, I'm not sure if it is really needed. @alongosz what do you think?

Should I implement something from things mentioned by @alongosz here: #1996 (comment)?

andrerom · 2017-05-30T13:59:52Z

Should I implement something from things mentioned by @alongosz here: #1996 (comment)?

Hmm, well you can, the second one is caused by tour new test, as this is depending on each other I suggest we end up with three commits that can be merged normally (not squashed):

commit to add testing of empty fields (several fields should fail on this commit alone it seems by the look of this PR and it's travis failures)
commit to fix date/time/dateAndTime where needed
commit to fix GeoLocationMapper::map

one separate PR for Symfony 3.3, @alongosz can you handle that one? (6.7 and up to unblock PR's)

alongosz · 2017-05-30T14:08:42Z

Was Time and DateAndTime actually affected by the timezone issue solved in #1942?

It wasn't AFAIR.
If we look at the original code for DateAndTime\SearchField and Time\SearchField, integers are indexed there which makes no sense for search engine. Code itself will work, as in PHP null['whatever'] silently results in null... (@Nattfarinn remindend me of this today :-))

Anyway, I thought this would be a good improvement, but it's probably a little bit out of scope of PR. One could also argue that it's a BC break, but it's hard for me to imagine someone actually relying on unix timestamps in search index ;)

alongosz · 2017-05-30T14:32:15Z

one separate PR for Symfony 3.3, @alongosz can you handle that one? (6.7 and up to unblock PR's)

#1998

kmadejski · 2017-05-30T14:41:56Z

@andrerom I discussed it with @alongosz and it seems to be unnecessary in FieldTypes other than Date so right now I kept changes only there. Added commit with test and also fix for GeoLocationMapper. Is it okay?

andrerom · 2017-05-30T15:00:41Z

@kmadejski Almost. 1. you have 6 commits and not 3 :) and 2. while at it rebase on 6.7 which now has fix for symfony 3.3.

slaci · 2017-06-01T08:57:19Z

Is that possible: this getIndexData is called for non searchable fields too on publish?

I have an ezdate field where searchable is 0 (I can see it in ezcontentclass_attribute table too its really 0), but publish fails with the invalidargument exception, cause the timestamp is null.

This only happens with solr, not with legacy search.

So currently ezdate cannot be optional :\

kmadejski · 2017-06-01T10:13:15Z

@slaci I checked it right now, content can be published without errors when is not Searchable no matter which search engine is used. I don't know what caused your problem. Anyway, this solution might be helpful also in the case which you described.

alongosz requested changes May 26, 2017

View reviewed changes

alongosz previously approved these changes May 26, 2017

View reviewed changes

andrerom previously approved these changes May 29, 2017

View reviewed changes

This was referenced May 29, 2017

[WIP] Fix EZP-27281: Fix BC on Date FieldType indexing #1967

Closed

Fix EZP-27281: Fix BC on Date FieldType indexing, add change in other… #1992

Closed

alongosz requested changes May 30, 2017

View reviewed changes

kmadejski force-pushed the EZP-27373_Error_when_edit_empty_not_required_date branch from 36ec684 to 1b62c5a Compare May 30, 2017 11:36

kmadejski force-pushed the EZP-27373_Error_when_edit_empty_not_required_date branch from 1b62c5a to d419113 Compare May 30, 2017 14:35

Fix-EZP-27373: Empty fields test

0ed31d5

kmadejski force-pushed the EZP-27373_Error_when_edit_empty_not_required_date branch from 86781e4 to aa5c035 Compare May 31, 2017 07:03

kmadejski added 2 commits May 31, 2017 10:37

Fix-EZP-27373: Fixed in Date FieldType

b79458f

Fix-EZP-27373: GeoLocationMapper::map fix

9712e75

kmadejski force-pushed the EZP-27373_Error_when_edit_empty_not_required_date branch from 2ec906f to 9712e75 Compare May 31, 2017 08:57

alongosz approved these changes May 31, 2017

View reviewed changes

andrerom approved these changes May 31, 2017

View reviewed changes

andrerom merged commit 89fe48c into ezsystems:6.7 Jun 1, 2017

kmadejski deleted the EZP-27373_Error_when_edit_empty_not_required_date branch June 20, 2018 12:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix-EZP-27373: Error when edit empty not required date #1996

Fix-EZP-27373: Error when edit empty not required date #1996

kmadejski commented May 26, 2017 •

edited

alongosz May 26, 2017

kmadejski May 26, 2017 •

edited

alongosz May 26, 2017

alongosz commented May 29, 2017

andrerom commented May 29, 2017

kmadejski commented May 29, 2017

alongosz commented May 29, 2017

kmadejski commented May 29, 2017 •

edited

andrerom commented May 29, 2017 •

edited

kmadejski commented May 29, 2017

andrerom commented May 29, 2017

kmadejski commented May 29, 2017

kmadejski commented May 29, 2017

alongosz left a comment

alongosz May 30, 2017

alongosz May 30, 2017

andrerom May 30, 2017 •

edited

alongosz May 30, 2017

alongosz May 30, 2017

alongosz commented May 30, 2017

kmadejski commented May 30, 2017

andrerom commented May 30, 2017 •

edited

alongosz commented May 30, 2017

andrerom commented May 30, 2017 •

edited

kmadejski commented May 30, 2017 •

edited

andrerom commented May 30, 2017

alongosz commented May 30, 2017

alongosz commented May 30, 2017

kmadejski commented May 30, 2017

andrerom commented May 30, 2017

slaci commented Jun 1, 2017 •

edited

kmadejski commented Jun 1, 2017

Fix-EZP-27373: Error when edit empty not required date #1996

Fix-EZP-27373: Error when edit empty not required date #1996

Conversation

kmadejski commented May 26, 2017 • edited

Description

alongosz May 26, 2017

Choose a reason for hiding this comment

kmadejski May 26, 2017 • edited

Choose a reason for hiding this comment

alongosz May 26, 2017

Choose a reason for hiding this comment

alongosz commented May 29, 2017

andrerom commented May 29, 2017

kmadejski commented May 29, 2017

alongosz commented May 29, 2017

kmadejski commented May 29, 2017 • edited

andrerom commented May 29, 2017 • edited

kmadejski commented May 29, 2017

andrerom commented May 29, 2017

kmadejski commented May 29, 2017

kmadejski commented May 29, 2017

alongosz left a comment

Choose a reason for hiding this comment

alongosz May 30, 2017

Choose a reason for hiding this comment

alongosz May 30, 2017

Choose a reason for hiding this comment

andrerom May 30, 2017 • edited

Choose a reason for hiding this comment

alongosz May 30, 2017

Choose a reason for hiding this comment

alongosz May 30, 2017

Choose a reason for hiding this comment

alongosz commented May 30, 2017

kmadejski commented May 30, 2017

andrerom commented May 30, 2017 • edited

alongosz commented May 30, 2017

andrerom commented May 30, 2017 • edited

kmadejski commented May 30, 2017 • edited

andrerom commented May 30, 2017

alongosz commented May 30, 2017

alongosz commented May 30, 2017

kmadejski commented May 30, 2017

andrerom commented May 30, 2017

slaci commented Jun 1, 2017 • edited

kmadejski commented Jun 1, 2017

kmadejski commented May 26, 2017 •

edited

kmadejski May 26, 2017 •

edited

kmadejski commented May 29, 2017 •

edited

andrerom commented May 29, 2017 •

edited

andrerom May 30, 2017 •

edited

andrerom commented May 30, 2017 •

edited

andrerom commented May 30, 2017 •

edited

kmadejski commented May 30, 2017 •

edited

slaci commented Jun 1, 2017 •

edited