Skip to content
This repository has been archived by the owner on Jan 8, 2020. It is now read-only.

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

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

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

akrabat opened this issue May 2, 2013 · 13 comments
Assignees
Milestone

Comments

@akrabat
Copy link
Contributor

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
Copy link
Member

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

@akrabat
Copy link
Contributor Author

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
Copy link
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.

@akrabat
Copy link
Contributor Author

akrabat commented May 2, 2013

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

@davidwindell
Copy link
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);

@akrabat
Copy link
Contributor Author

akrabat commented May 2, 2013

Thanks @davidwindell - PR created.

@akrabat akrabat closed this as completed in 812ea7e May 2, 2013
weierophinney added a commit that referenced this issue May 2, 2013
@ghost ghost assigned akrabat May 2, 2013
@DASPRiD
Copy link
Member

DASPRiD commented May 2, 2013

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

@davidwindell
Copy link
Contributor

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

@gws
Copy link
Contributor

gws commented May 2, 2013

0 is a perfectly valid UNIX timestamp.

@davidwindell
Copy link
Contributor

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

@akrabat
Copy link
Contributor Author

akrabat commented May 3, 2013

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

@davidwindell
Copy link
Contributor

: thumbsup

@gws
Copy link
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
This addresses the note raised in issue zendframework/zendframework#4393 after merge that zero is a
valid UNIX timestamp.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants