[HttpFoundation] adds getDate method to the ParameterBag class #19296
Conversation
|
You can rebase your commit history and |
| public function getDate($key, $format = 'Y-m-d', $default = null, $timeZone = null) | ||
| { | ||
| if (!$this->has($key)) { | ||
| return $default; |
There was a problem hiding this comment.
This gives no guaranties whatsoever about the type of the return value ($default can be anything).
Should default be a plain string that is turned to a date instead?
There was a problem hiding this comment.
Could very well be, I like this more than manually constructing a DateTime object for the default parameter.
So you can call it as follows: $bag->getDate('from', 'Y-m-d', '2016-01-01')
Yeah, perhaps we can go this way.
There was a problem hiding this comment.
Why not typehint $default + $timezone? Fixes violating the return type :)
…converted to a datetime; added more tests
| */ | ||
| public function getDate($key, $format, $default = null, $timeZone = null) | ||
| { | ||
| $time = $this->get($key, (string) $default); |
There was a problem hiding this comment.
What about
if (null === ($time = $this->get($key, $default))) {
return null;
}Next, if $format is invalid (ie false === $result) i would expect a \LogicException or something.
There was a problem hiding this comment.
Not too sold on creating a variable and checking for value in the same line, however that is a matter of preference.
Early return might be doable, but I avoided it because fabbot doesn't accept return null; but wants just a return; which technically both return null but I still see it as a void vs null issue.
Regarding \LogicException: None of the methods of this class throws an exception for invalid input. (e.g.: getInt rather than checking if it's a valid number or not it simply type-casts it).
There was a problem hiding this comment.
Do it in 2 lines then :) Its more about not casting $default, and rely on whatever value $time is. (null, valid or invalid).
Hence i would throw an exception when the value is invalid for the given format. Ignoring the user's expected format and just return null can be a real WTF. This is also the difference compared to getInt, etc. However it can also be explained as the user responsibility to validate (still we cannot differentiate between invalid or default/null) , but imo it would be the added value for this shortcut ;-)
Separating null from void is a good thing :) And this should be accepted by now afaik (symfony/symfony-docs#6614)
There was a problem hiding this comment.
I agree about not casting $default
|
👍 Status: Reviewed |
|
👍 |
| @@ -133,7 +161,7 @@ public function testFilter() | |||
| 'dec' => '256', | |||
| 'hex' => '0x100', | |||
| 'array' => array('bang'), | |||
| )); | |||
| )); | |||
There was a problem hiding this comment.
This cs fix should be done in 2.7, not in master, right?
There was a problem hiding this comment.
The cs change was pushed accidentaly. Probably should be reverted.
|
Currently the default date is ignored whenever the user date is given but cannot be parsed (ie garbage). Im not sure thats convenient... However i can imagine this heavily depends on what the end user wants/needs. Which probably also counts for exception handling on invalid date (vs returning null). Maybe this behaviour should be toggable after all via arguments =/
|
| { | ||
| $time = $this->get($key, $default); | ||
|
|
||
| if ((null === $time) || $time instanceof \DateTimeInterface) { |
There was a problem hiding this comment.
(null === $time) parentheses unnecessary ?
|
So, given the amount of time/discussion we spent here, given that error handling is a domain matter, I'm not sure we want to add this anymore. The boilerplate it saves looks just better that creating a method with too many arguments and complexity inside. |
|
I'd say if this was to be merged it should be merged as it currently is. I'm not okay with defining behaviour via arguments. If symfony prefers to throw exceptions then this should also throw otherwise if symfony prefers to return null then so be it. |
|
@maldoinc what's the benefit of adding that to the parameter bag? It's something that the serializer for example can already do: $dateTime = $serializer->denormalize($parameters->get($key), \DateTime::class);Sure it is slightly less convenient, but that's something serializer has been designed for whereas I hardly see why a ParameterBag should try to transform a value into an object. |
|
Consider public function getDate($key, $format, $default = null, \DateTimeZone $timeZone = null)
{
if (null !== $default && !$default instanceof \DateTimeInterface) {
$default = null === $timeZone ? \DateTime::createFromFormat($format, $default) : \DateTime::createFromFormat($format, $default, $timeZone);
if (false === $default) {
throw new \InvalidArgumentException('Invalid default date given');
}
}
$time = $this->get($key, $default);
if (null === $time || $time instanceof \DateTimeInterface) {
return $time;
}
$result = null === $timeZone ? \DateTime::createFromFormat($format, $time) : \DateTime::createFromFormat($format, $time, $timeZone);
return false === $result ? $default : $result;
}I'd vote for that. |
| // this is in order to ensure compatibility with all php versions since | ||
| // some accept null as a TimeZone parameter and others do not | ||
| if (null !== $timeZone) { | ||
| $result = \DateTime::createFromFormat($format, $time, $timeZone); |
There was a problem hiding this comment.
If format is U, the timezone won't be applied.
|
I think I agree with @nicolas-grekas and @theofidry, and I think we should not add this to the core. |
|
Ok, let's close here then per @nicolas-grekas' comment. |
This adds a
getDatemethod to theParameterBagclass which convert the specified key to aDateTimeobjectEdit: The main motivation behind this PR (from my PoV at least) is to avoid repetitive tasks with handling date parameters on GET requests.
This is a continuation of #19295. Moved here in order to have a cleaner git history.