-
Notifications
You must be signed in to change notification settings - Fork 47
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix Array persistence with Datetime fields #739
Fix Array persistence with Datetime fields #739
Conversation
$m->save(); | ||
|
||
//try setting DateTime Object | ||
$dateTime = \DateTime::createFromFormat('Y-m-d H:i:s', '2020-01-01 11:11:11'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new \DateTime('2020-01-01 11:11:11')
is enough
$m->set('datetime', $dateTime); | ||
$m->save(); | ||
|
||
self::assertInstanceOf( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
keep on 1 line, also, probably use $this->assert
to be consistent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does StyleCI command this? I find assertions way more readable this way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 line - keep files short - important in general
valid usecase - related with #727 we should typecase inside Field to an unmanaged object/string and twice (to/from) for normalize probably also related with #690 UPDATE: #690 does not solve it, array persistence simply does not care about typecasting, which is wrong, as object can not be compared at all (in sense of non strict equality) and typecasting/normalize thru field Type seems to be the only correct approach |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should aim to deduplicate tests and tests things like typecasting for every Persistence from one file.
Yes, this sounds reasonable. In general I am not happy adding another typecasting to this persistence. You and Georgi already put a lot of thought in this topic, and as far as I understood you have some good idea how to fix typecasting issues on a different level. |
Is this still valid with #804 ? |
Currently, the added test fails and produces this error message:
The objective of this PR is to fix this issue. As no fix is provided yet, just a draft.