2.2RC1 BC Break: DateTimeFormatter sets blank data to today's date #4393

Closed
akrabat opened this Issue May 2, 2013 · 13 comments

Comments

Projects
None yet
5 participants
@akrabat
Member

akrabat commented May 2, 2013

Updating my project to 2.2 RC1, I find that any Zend\Form\Element\Date element can now no longer be empty as today's date is returned from the automatically added DateTimeFormatter filter...

@weierophinney

This comment has been minimized.

Show comment
Hide comment
@weierophinney

weierophinney May 2, 2013

Member

Are you working on a fix yet? (Don't want to duplicate effort!)

Member

weierophinney commented May 2, 2013

Are you working on a fix yet? (Don't want to duplicate effort!)

@akrabat

This comment has been minimized.

Show comment
Hide comment
@akrabat

akrabat May 2, 2013

Member

Not sure of how complex a fix is required. The most simplistic fix is:

diff --git a/library/Zend/Filter/DateTimeFormatter.php b/library/Zend/Filter/DateTimeFormatter.php
index f0dee58..19a9e6c 100644
--- a/library/Zend/Filter/DateTimeFormatter.php
+++ b/library/Zend/Filter/DateTimeFormatter.php
@@ -71,7 +71,9 @@ class DateTimeFormatter extends AbstractFilter
      */
     protected function normalizeDateTime($value)
     {
-        if (is_int($value)) {
+        if (empty($value)) {
+            return '';
+        } elseif (is_int($value)) {
             $dateTime = new DateTime('@' . $value);
         } elseif (!$value instanceof DateTime) {
             $dateTime = new DateTime($value);

But, I'm unsure whether we should introduce an option for this.

Member

akrabat commented May 2, 2013

Not sure of how complex a fix is required. The most simplistic fix is:

diff --git a/library/Zend/Filter/DateTimeFormatter.php b/library/Zend/Filter/DateTimeFormatter.php
index f0dee58..19a9e6c 100644
--- a/library/Zend/Filter/DateTimeFormatter.php
+++ b/library/Zend/Filter/DateTimeFormatter.php
@@ -71,7 +71,9 @@ class DateTimeFormatter extends AbstractFilter
      */
     protected function normalizeDateTime($value)
     {
-        if (is_int($value)) {
+        if (empty($value)) {
+            return '';
+        } elseif (is_int($value)) {
             $dateTime = new DateTime('@' . $value);
         } elseif (!$value instanceof DateTime) {
             $dateTime = new DateTime($value);

But, I'm unsure whether we should introduce an option for this.

@weierophinney

This comment has been minimized.

Show comment
Hide comment
@weierophinney

weierophinney May 2, 2013

Member

I'd argue if it was the original behavior, we should probably add a test and make the change. Check through the changelog to see what may have prompted the change in behavior, though; if it was on purpose, we should add an option.

Member

weierophinney commented May 2, 2013

I'd argue if it was the original behavior, we should probably add a test and make the change. Check through the changelog to see what may have prompted the change in behavior, though; if it was on purpose, we should add an option.

@akrabat

This comment has been minimized.

Show comment
Hide comment
@akrabat

akrabat May 2, 2013

Member

PR that introduced DateTimeFormatter is #3632 . Any thoughts on this @davidwindell ?

Member

akrabat commented May 2, 2013

PR that introduced DateTimeFormatter is #3632 . Any thoughts on this @davidwindell ?

@davidwindell

This comment has been minimized.

Show comment
Hide comment
@davidwindell

davidwindell May 2, 2013

Contributor

@akrabat I clearly missed handling for empty values and agree we need to do so, I would be against returning '', instead simply return $value if it isempty. In an API scenario one may be handling null values, not just empty strings.

     protected function normalizeDateTime($value)
     {
-        if (is_int($value)) {
+        if (empty($value)) {
+            return $value;
+        } elseif (is_int($value)) {
             $dateTime = new DateTime('@' . $value);
         } elseif (!$value instanceof DateTime) {
             $dateTime = new DateTime($value);
Contributor

davidwindell commented May 2, 2013

@akrabat I clearly missed handling for empty values and agree we need to do so, I would be against returning '', instead simply return $value if it isempty. In an API scenario one may be handling null values, not just empty strings.

     protected function normalizeDateTime($value)
     {
-        if (is_int($value)) {
+        if (empty($value)) {
+            return $value;
+        } elseif (is_int($value)) {
             $dateTime = new DateTime('@' . $value);
         } elseif (!$value instanceof DateTime) {
             $dateTime = new DateTime($value);
@akrabat

This comment has been minimized.

Show comment
Hide comment
@akrabat

akrabat May 2, 2013

Member

Thanks @davidwindell - PR created.

Member

akrabat commented May 2, 2013

Thanks @davidwindell - PR created.

@akrabat akrabat closed this in 812ea7e May 2, 2013

weierophinney added a commit that referenced this issue May 2, 2013

@ghost ghost assigned akrabat May 2, 2013

@DASPRiD

This comment has been minimized.

Show comment
Hide comment
@DASPRiD

DASPRiD May 2, 2013

Member

Wait wait, not empty(), do a === '' comparision!

Member

DASPRiD commented May 2, 2013

Wait wait, not empty(), do a === '' comparision!

@davidwindell

This comment has been minimized.

Show comment
Hide comment
@davidwindell

davidwindell May 2, 2013

Contributor

Why? I would never want an empty value normalised to today's date.

Contributor

davidwindell commented May 2, 2013

Why? I would never want an empty value normalised to today's date.

@gws

This comment has been minimized.

Show comment
Hide comment
@gws

gws May 2, 2013

Contributor

0 is a perfectly valid UNIX timestamp.

Contributor

gws commented May 2, 2013

0 is a perfectly valid UNIX timestamp.

@davidwindell

This comment has been minimized.

Show comment
Hide comment
@davidwindell

davidwindell May 3, 2013

Contributor

Ooh yeah, @akrabat can you do a check for "" and null instead please?

Contributor

davidwindell commented May 3, 2013

Ooh yeah, @akrabat can you do a check for "" and null instead please?

@akrabat

This comment has been minimized.

Show comment
Hide comment
@akrabat

akrabat May 3, 2013

Member

Would people prefer if ($value === '' || $value === null) { ?

Member

akrabat commented May 3, 2013

Would people prefer if ($value === '' || $value === null) { ?

@davidwindell

This comment has been minimized.

Show comment
Hide comment
@davidwindell

davidwindell May 3, 2013

Contributor

: thumbsup

Contributor

davidwindell commented May 3, 2013

: thumbsup

@gws

This comment has been minimized.

Show comment
Hide comment
@gws

gws May 3, 2013

Contributor

👍

Contributor

gws commented May 3, 2013

👍

gianarb pushed a commit to zendframework/zend-filter that referenced this issue May 15, 2015

weierophinney added a commit to zendframework/zend-filter that referenced this issue May 15, 2015

gianarb pushed a commit to zendframework/zend-filter that referenced this issue May 15, 2015

Allow DateTimeFormatter to format zero.
This addresses the note raised in issue zendframework/zendframework#4393 after merge that zero is a
valid UNIX timestamp.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment