-
Notifications
You must be signed in to change notification settings - Fork 2.5k
Created DateTime Formater strategy for hydrator #6289
Created DateTime Formater strategy for hydrator #6289
Conversation
*/ | ||
public function extract($value) | ||
{ | ||
if ($value === null) { |
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.
Not needed as if value is null it will be returned at the end of the method.
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.
I am stupid. Thanks.
*/ | ||
public function extract($value) | ||
{ | ||
if ($value instanceof DateTime) { |
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.
What about timezone handling, for both extraction and hydration? There should be a way to define the timezone of the string representation for both extraction and hydration, in case the datetime format does not define a timezone.
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.
@bakura10 Any idea how?
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.
What about only returning $value
only if the format function returns false? This would allow integer, and strings to be formatted also?
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.
I'm not sure about the timezone. Can't you set the timezone globally ? iirc format uses the server timezone, isn't it what we want in general?
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.
@devosc , good idea, you could add a check for int:
public function extract($value) {
if (is_int($value)) {
$timestamp = $value;
$value = new DateTime();
$value->setTimestamp($timestamp);
}
}
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.
To give a concrete example, why createFromFormat must know the timezone (note: the time given by a user my differ from the system timezone):
2014-10-26 02:30:00
can define two different points in time in the Europe/Berlin timezone, thanks to that stupid invention called DST. Thus it is required to know whether it is defined as CET or CEST.
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.
@DASPRiD What about this way?
For hydration:
public function hydrate($value)
{
if ($value === '' || $value === null) {
return null;
}
return DateTime::createFromFormat($this->format, $value, $this->hydrateTimeZone);
}
For extraction:
public function extract($value)
{
if ($value instanceof DateTime) {
if ($this->extractTimeZone instance DateTimeZone) {
$value->setTimezone($this->extractTimeZone);
}
return $value->format($this->format);
}
return $value;
}
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.
I don't see a concrete reason to have two different timezones for extraction and hydration. A hydrator should always work synchronous in both way. If the extraction timezone would differ from the hydration timezone, then the following would be false, which is basically wrong:
$value === $strategy->extract($strategy->hydrate($value));
Apart from that, it sounds fine.
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.
@DASPRiD I have done some work for timezone. Please check out.
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.
Looks good.
Looks good to me, @weierophinney , that should be ready for merging :). |
+1 |
All is well |
There is a method |
* @param DateTimeZone $timezone | ||
* @return void | ||
*/ | ||
public function setTimezone(DateTimeZone $timezone) |
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.
Are the setters required?
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.
I think so. timezone is optional.
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.
It's still a constructor argument, so I think it doesn't need a setter
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.
Ok. I will remove it.
What's the status of this? If it's ready to go, could you rebase and squash the commits? |
a6eca3b
to
a88eb48
Compare
@adamlundrigan Thanks for reminding. I have rebased and squashed the commits. |
I've made the class
|
…ould accept only string-castable formats
…t in constructor
…TimeFormatterStrategy`
…TimeFormatterStrategy`
… internal implementation details `private`
… ignore string values that cannot be hydrated
… ignore string values that cannot be hydrated
…coverage annotations
…tter-strategy' into develop Close zendframework/zendframework#6289
I screwed up PR #6198
Removed
Zend\Filter
dependency.