Allow extended ISO datetime format to be marshalled. #10587
Conversation
Plus remove unused variable.
Correct and improve doc block of RouteCollection::match()
src/Database/Type/DateTimeType.php
Outdated
| @@ -114,7 +117,8 @@ public function toDatabase($value, Driver $driver) | |||
| $value = new $class('@' . $value); | |||
| } | |||
|
|
|||
| return $value->format($this->_format); | |||
| $format = (array)$this->_format; | |||
| return $value->format(array_shift($format)); | |||
There was a problem hiding this comment.
Missing blank line before return statement
src/Database/Type/DateTimeType.php
Outdated
| @@ -206,6 +210,22 @@ public function marshal($value) | |||
| } | |||
|
|
|||
| /** | |||
| * @param \Cake\I18n\Time|\DateTime $date | |||
There was a problem hiding this comment.
Missing parameter comment
dereuromark
changed the title
src/Database/Type/DateTimeType.php
Outdated
| protected $_format = 'Y-m-d H:i:s'; | ||
| protected $_format = [ | ||
| 'Y-m-d H:i:s', | ||
| 'Y-m-d\TH:i:s.uO', |
There was a problem hiding this comment.
This will change the accepted data of every application using CakePHP. Not sure that is something we should be doing in a bugfix release.
There was a problem hiding this comment.
It should be fully BC as it only adds on top. But sure, we can also either make it opt-in from app level, or do it in a minor.
Maybe there are also even better options here than this.
There was a problem hiding this comment.
My concern is more that this adds a new valid input form that was not previously accepted. Developers may be surprised when previously invalid data becomes valid. Also concerning is that the new format includes timezone information which will require apps to ensure they are doing timezone conversions.
There was a problem hiding this comment.
Actually i ran into this issue because we started testing with MySQL 5.7, and the same app/db on MySQL 5.6 this "just works" (posting json from a js frontend to a controller)
There was a problem hiding this comment.
Yes that was my Pain Point too.
5.6 works fine, 5.7 crashes :/
There was a problem hiding this comment.
Are you saying that 5.7 fails to insert rows with the current datetime format marshalling?
There was a problem hiding this comment.
MySQL 5.6:
mysql> insert into `products` set `created` = '2017-05-02T08:30:44+00:00';
Query OK, 1 row affected, 1 warning (0.00 sec)MySQL 5.7:
mysql> insert into `products` set `created` = '2017-05-02T08:30:44+00:00';
ERROR 1292 (22007): Incorrect datetime value: '2017-05-02T08:30:44+00:00' for column 'created' at row 1There was a problem hiding this comment.
Ok. So having the ISO formats marshalled into datetime objects would likely help with that.
| @@ -165,7 +169,7 @@ public function marshal($value) | |||
| $date = new $class($value); | |||
| $compare = true; | |||
| } | |||
| if ($compare && $date && $date->format($this->_format) !== $value) { | |||
| if ($compare && $date && !$this->_compare($date, $value)) { | |||
There was a problem hiding this comment.
Do we really need to check whether the date string can be restored here? I think this kind of check should be done by validation. If we could remove this check, people could use <input type="datetime-local"> field, that uses another date string formats, such as Y-m-d\TH:i or Y-m-d\TH:i:s.
Also, I think we have to change Validation::datetime() too. At the moment, it doesn't support for ISO datetime strings.
There was a problem hiding this comment.
Do you have an alternative PR for your recommendation? I am open to any other approach that would fix the issue.
There was a problem hiding this comment.
If we could get a consensus, I'll do that. Now I have a tricky way to pass the validation in my 2.x project:
public function isDateTime($check) {
$value = array_values($check)[0];
$value = implode(' ', explode('T', $value, 2));
return Validation::datetime($value);
}But it would be nice if Cake supports modern HTML features without this kind of trick.
There was a problem hiding this comment.
This check was originally intended to ensure that an ambiguous input format wasn't used resulting in a different datetime value than was in the request data. Perhaps that isn't useful, an we should instead rely on validation and app logic.
There was a problem hiding this comment.
OK. Then I'll send an alternative PR to 3.next when I have time. If it becomes to accept new date formats, the master branch would not be suitable.
There was a problem hiding this comment.
@chinpei215 I consider this more a bugfix, because so far this kind of ISO date was wrongly not supported.
Are there any reasonable downsides of those "valid" date strings to be not turned down? Just saying "because so far wasnt allowed" is not a valid reason. Those already get exported like this for years, they must also be able get imported the same way.
I do not think this puts existing apps in jeopardy.
There was a problem hiding this comment.
@dereuromark Changing string variable to array is not backward compatible. If someone uses a sub-class using _format property directly, the application would break. So this change must be done in 3.next.
|
what are the chances this could get looked at again ? anything i can do ? |
|
@darxmac Do you have an idea of implementation that would work better than this one? It seems the compare part here causes some issues. The tests do not pass. |
|
Looks like the format string was off a bit: |
update allowed date format string to match `ATOM` format
Codecov Report
@@ Coverage Diff @@
## 3.next #10587 +/- ##
============================================
- Coverage 94.96% 94.91% -0.05%
+ Complexity 12326 12116 -210
============================================
Files 427 422 -5
Lines 30714 30100 -614
============================================
- Hits 29168 28570 -598
+ Misses 1546 1530 -16
Continue to review full report at Codecov.
|
|
Looks like the tests are now green. |
|
Moved to 3.next as this is a bigger change than I think we should be putting into a bug fix release. |
|
@lorenzo What do you think? |
|
bump |
|
anything else i could do to help move this along ? |
| */ | ||
| protected function _compare($date, $value) | ||
| { | ||
| foreach ((array)$this->_format as $format) { |
There was a problem hiding this comment.
Do the other subclasses need any changes?
There was a problem hiding this comment.
Not that I could see.
|
@darxmac The earliest this will ship is 3.5.0 which hasn't even started beta/rc releases, so it will be a while even if this is merged today. |
| protected $_format = 'Y-m-d H:i:s'; | ||
| protected $_format = [ | ||
| 'Y-m-d H:i:s', | ||
| 'Y-m-d\TH:i:sP', |
There was a problem hiding this comment.
Could we add Y-m-d\TH:i:s.u to that list?
That would allow support for datetime(6) see: #10439
Edit: And then also for class TimeType ?
There was a problem hiding this comment.
Sure. You want to make a PR against this branch?
Not sure when I will be able to follow up here.
|
We could also already merge this and further enhance with future on top PRs? |
Implements #10586
A first stab at the issue.
I would only recommend adding the single one ISO format
'Y-m-d\TH:i:s.uO'which CakePHP generates on output in JSON files, so when doing the reverse, import, it can safely and directly convert back into the DB format.Any better idea on the subject?