10000 [HttpFoundation] adds getDate method to the ParameterBag class by maldoinc · Pull Request #19296 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[HttpFoundation] adds getDate method to the ParameterBag class #19296

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.

Sign up for GitHub

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

Closed
wants to merge 6 commits into from
Closed

[HttpFoundation] adds getDate method to the ParameterBag class #19296

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
319b955
added getDate/getDateTime methods to the parameterbag class
maldoinc Jul 5, 2016
ba3af51
removed getDateTime method; $default param is now a string wich gets…
maldoinc Jul 6, 2016
e7b221b
added type hinting for $timezone parameter. Removed cast to string on…
maldoinc Jul 17, 2016
e31f24b
added support for \DateTimeInterface default
maldoinc Jul 29, 2016
969b4ba
fixed test case to not take into account time on getDate tests
maldoinc Jul 29, 2016
b3a292e
early return includes null condition. added phpdoc
maldoinc Aug 11, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
removed getDateTime method; $default param is now a string wich gets … 
…converted to a datetime; added more tests
  • Loading branch information
@maldoinc
maldoinc committed Jul 6, 2016
commit ba3af51bd93edfdda57e0c4500f2da9cb81b3335
29 changes: 5 additions & 24 deletions src/Symfony/Component/HttpFoundation/ParameterBag.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,25 +192,21 @@ public function getBoolean($key, $default = false)
*
* @param string $key The parameter key
* @param string $format The expected date format
* @param \DateTime|null $default The default value if the parameter key does not exist
* @param \DateTimeZone|null $timeZone
* @param string $default The default value to be converted to a DateTime object if the parameter key does not exist
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not passing a DateTime object as the default value?

Copy link
Contributor Author
@maldoinc maldoinc Jul 7, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would argue that having a string to be returned as a date feels more natural and reuses existing parameters, also as per @nicolas-grekas suggestion.

Take the following examples. I would argue that the second one is more verbose and redundant as the timezone needs to be specified twice, which is why I ended up with a string as the default (in the original PR the default parameter was actually a datetime object as you suggest).

 $fromDate = $bag->getDate('fromDate', 'Y-m-d H:i:s', '2016-01-01 14:30:00', new \DateTimeZone('CET'));

$timezone = new \DateTimeZone('CET')
$default = new \DateTime('2016-01-01 14:30:00', $timezone);
$fromDate = $bag->getDate('fromDate', 'Y-m-d',  $default, $timezone);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xabbuh I stick to my first pov: all the other getter (getAlpha, etc.) start with something like $this->get($key, $default) then do transform the result.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, let's keep it this way to be consistent with the other methods.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though, the other getter methods accept mixed argument which works if you only do a simple type cast (for ints, for example, as an int cast to int is still an int). So I think we should still allow (but not require) to pass a \DateTime object here and simply not transform it when it is used.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@maldoinc wdyt of @xabbuh 's idea, allowing a DateTime(Interface) here?

10000
Copy link
Contributor Author
@maldoinc maldoinc Jul 26, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we can allow it for the $default parameter, although the combination of a \DateTimeInterface and a string is an odd one.

I'm also a bit torn on the error handling aspect.

Usually I'd expect the function to throw an exception on malformed input. However returning fasly values is more PHP-esque(?) @ro0NL also seems to be on this train. What's your input on this: @nicolas-grekas @xabbuh?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

returning null in case of parse failure looks fine to me: this is user input, ie potentially garbage. An exception would force a mandatory try/catch around any call to getDate (which most will forget). Doing if ($date = $foo->getDate(..)) has less boilerplate to me.

If we accept DateTimeInterface as default, then the return type hint should be changed to DateTimeInterface|null

* @param \DateTimeZone|null $timeZone A DateTimeZone object representing the desired time zone
*
* @return \DateTime|null
*/
public function getDate($key, $format = 'Y-m-d', $default = null, $timeZone = null)
public function getDate($key, $format, $default = null, $timeZone = null)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still not sure why $timeZone is not typehinted...

{
if (!$this->has($key)) {
return $default;
}

$time = $this->get($key);
$time = $this->get($key, (string) $default);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author
@maldoinc maldoinc Jul 6, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor
@ro0NL ro0NL Jul 6, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree about not casting $default


// if the user has specified a timezone then pass that
// otherwise do not even attempt to put a value but rather let the runtime decide
// the default value by itself
// this is in order to ensure compatibility with all php versions since
// some accept null as a TimeZone parameter and others do not
if ($timeZone !== null) {
if (null !== $timeZone) {
$result = \DateTime::createFromFormat($format, $time, $timeZone);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If format is U, the timezone won't be applied.

} else {
$result = \DateTime::createFromFormat($format, $time);
Expand All @@ -220,21 +216,6 @@ public function getDate($key, $format = 'Y-m-d', $default = null, $timeZone = nu
return false === $result ? null : $result;
Copy link
Contributor
@ro0NL ro0NL Aug 6, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just thought of this.. what about one last argument $throwOnInvalid. Basically to avoid boilerplate code in controllers, etc. (ie null checking). Defaulting to false is just fine.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just read @nicolas-grekas comment once more

returning null in case of parse failure looks fine to me: this is user input, ie potentially garbage

I guess your right. Letting the app error 500 based on user input is not desired. Maybe it should only throw InvalidArgumentException when $default as a string is invalid.. wdyt?

}

/**
* Returns the parameter value converted to a DateTime object while also parsing the time.
*
* @param string $key The parameter key
* @param string $format The expected date format
* @param \DateTime|null $default The default value if the parameter key does not exist
* @param \DateTimeZone|null $timeZone
*
* @return \DateTime|null
*/
public function getDateTime($key, $format = 'Y-m-d H:i:s', $default = null, \DateTimeZone $timeZone = null)
{
return $this->getDate($key, $format, $default, $timeZone);
}

/**
* Filter key.
*
Expand Down
19 changes: 7 additions & 12 deletions src/Symfony/Component/HttpFoundation/Tests/ParameterBagTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,15 +124,6 @@ public function testGetInt()
$this->assertEquals(0, $bag->getInt('unknown'), '->getInt() returns zero if a parameter is not defined');
}

public function testGetDateTime()
{
$format = 'Y-m-d H:i:s';
$bag = new ParameterBag(array('d1' => '2016-01-01 00:00:00'));
$date = \DateTime::createFromFormat($format, '2016-01-01 00:00:00');

$this->assertEquals($date, $bag->getDateTime('d1', $format), '->getDateTime() returns a date from the specified format');
}

public function testGetDate()
{
$isoDate = '2016-07-05T15:30:00UTC';
Expand All @@ -142,17 +133,21 @@ public function testGetDate()
));

$date = \DateTime::createFromFormat('Y-m-d', '2016-01-01');
$diff = $date->diff($bag->getDate('d1'));
$diff = $date->diff($bag->getDate('d1', 'Y-m-d'));

$this->assertEquals(0, $diff->days, '->getDate() returns a date via the format specified');
$this->assertNull($bag->getDate('d1', 'd/m/Y'), '->getDate() returns false if the format is not valid');
$this->assertNull($bag->getDate('d1', 'd/m/Y'), '->getDate() returns null if the format is not valid');
$this->assertNull($bag->getDate('d2', 'd/m/Y'), '->getDate() returns null if the parameter is not found');

$date = $bag->getDate('iso', \DateTime::ISO8601);
$this->assertEquals(new \DateTime($isoDate), $date);
$this->assertEquals('UTC', $date->getTimezone()->getName());

$this->assertEquals($date, $bag->getDate('nokey', 'Y-m-d', $date));
$this->assertEquals($date, $bag->getDate('nokey', \DateTime::ISO8601, $isoDate));
$this->assertNull($bag->getDate('nokey', 'd/m/Y', $isoDate), '->getDate() returns null when the default value is not in the specified format');

$tz = $bag->getDate('d1', 'Y-m-d', null, new \DateTimeZone('Europe/Tirane'))->getTimezone()->getName();
$this->assertEquals('Europe/Tirane', $tz, '->getDate() accepts a DateTimeZone object which specifies the preferred timezone');
}

public function testFilter()
Expand Down
0