-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[HttpKernel] Add timezone to DateTimeValueResolver #45865
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
Conversation
bd7e27a
to
b07747f
Compare
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 adding the default TZ as constructor argument to the resolver also? (and maybe add a config option?)
src/Symfony/Component/HttpKernel/Controller/ArgumentResolver/DateTimeValueResolver.php
Show resolved
Hide resolved
The default TZ is already configured on the system. |
@codedmonkey you worked on this feature; could you review? |
If it makes sense to hardcode a TZ in the code (thus overriding the ini setting, which is what this PR allows doing), then I suppose it makes sense to allow hardcoding the default TZ in the config, don't you think? |
I feel like hardcoding a TZ in annotations doesn't really make sense. Instead, shouldn't we patch the source like this? --- a/src/Symfony/Component/HttpKernel/Controller/ArgumentResolver/DateTimeValueResolver.php
+++ b/src/Symfony/Component/HttpKernel/Controller/ArgumentResolver/DateTimeValueResolver.php
@@ -64,8 +64,12 @@ final class DateTimeValueResolver implements ArgumentValueResolverInterface
}
} elseif (false !== filter_var($value, \FILTER_VALIDATE_INT, ['options' => ['min_range' => 0]])) {
$date = new $class('@'.$value);
- } elseif (false !== $timestamp = strtotime($value)) {
- $date = new $class('@'.$timestamp);
+ } else {
+ try {
+ $date = new $class($value);
+ } catch (\Exception $e) {
+ $date = false;
+ }
} |
Yes, that makes sense and solves the main issue. |
Implemented in a new PR: #45874 |
…e (GromNaN) This PR was merged into the 6.1 branch. Discussion ---------- [HttpKernel] Resolve DateTime values with default timezone | Q | A | ------------- | --- | Branch? | 6.1 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | #45865 (comment) | License | MIT | Doc PR | - Alternative to #45865 by Nicolas Grekas. Make sure the date input is parsed using the default timezone and the resulting `DateTimeInterface` instance has the default timezone. Except for timestamps, which are UTC by standard. Commits ------- 7d17928 Resolve DateTime values with default timezone
Motivation:
This was a source of confusion and bugs when working with SensioFrameworkExtraBundle's
DateTimeParamConverter
(3rd case is default and worse behavior).$format
specified:DateTime::fromFormat($format, $value)
parses the date using the default timezone. The output has the timezone specified in$value
(and expected by$format
), or the default timezone.new DateTime('@'.$timestamp)
parses the timestamp in UTC. The output has the timezone UTC.new DateTime('@'.strtotime($value))
parses the date using the default timezone. The output has the timezone UTC. This is confusing.Proposed change
The new option
timezone
forces the timezone of parser and output.Why not just set the default timezone? Because it can be different from one argument to an other.
This option is also useful to specify output timezone while requiring a timezone in the input format.
The special value
timezone: 'default'
will get the current default timezone fromdate_default_timezone_get()
.