8000 [HttpKernel] Add timezone to DateTimeValueResolver by GromNaN · Pull Request #45865 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

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

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

[HttpKernel] Add timezone to DateTimeValueResolver #45865

wants to merge 1 commit into from

Conversation

GromNaN
Copy link
Member
@GromNaN GromNaN commented Mar 27, 2022 •
edited
Loading
Q A
Branch? 6.1
Bug fix? no
New feature? yes
Deprecations? no
Tickets -
License MIT
Doc PR todo

Motivation:

This was a source of confusion and bugs when working with SensioFrameworkExtraBundle's DateTimeParamConverter (3rd case is default and worse behavior).

  1. $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.
  2. Timestamp: new DateTime('@'.$timestamp) parses the timestamp in UTC. The output has the timezone UTC.
  3. Otherwise: 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.

    public function __invoke(
        #[MapDateTime(timezone: 'Antarctica/Troll')] \DateTimeInterface $date
    )
    // 2022-06-01 08:10:00 becomes 2022-06-01 08:10:00.0 Antarctica/Troll (+02:00)
    // 2022-06-01 08:10:00+05:00 becomes 2022-06-01 05:10:00.0 Antarctica/Troll (+02:00)

    public function __invoke(
        \DateTimeInterface $date
    )
    // 2022-06-01 08:10:00 becomes 2022-06-01 06:10:00.0 +00:00
    // 2022-06-01 08:10:00+05:00 becomes 2022-06-01 03:10:00.0 +00:00

Why not just set the default timezone? Because it can be different from one argument to an other.

    public function __invoke(
        #[MapDateTime(timezone: 'Antarctica/Troll')] \DateTimeInterface $localDate
        #[MapDateTime(timezone: 'UTC')] \DateTimeInterface $universalDate
    )

This option is also useful to specify output timezone while requiring a timezone in the input format.

    public function __invoke(
        #[MapDateTime('Y-m-d H:i:sP', timezone: 'UTC')] \DateTimeInterface $date
    )
    // 2022-06-01 08:10:00+05:00 becomes 2022-06-01 03:10:00.0 +00:00

    public function __invoke(
        #[MapDateTime('Y-m-d H:i:sP')] \DateTimeInterface $date
    )
    // 2022-06-01 08:10:00+05:00 becomes 2022-06-01 08:10:00 +05:00

The special value timezone: 'default' will get the current default timezone from date_default_timezone_get().

@carsonbot carsonbot added this to the 6.1 milestone Mar 27, 2022
@GromNaN GromNaN force-pushed the param-converter-tz branch from bd7e27a to b07747f Compare March 27, 2022 20:50
nicolas-grekas
nicolas-grekas reviewed Mar 28, 2022
View reviewed changes
Copy link
Member
@nicolas-grekas nicolas-grekas left a 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?)

@GromNaN
Copy link
Member Author
GromNaN commented Mar 28, 2022

What about adding the default TZ as constructor argument to the resolver also? (and maybe add a config option?)

The default TZ is already configured on the system.

@GromNaN
Copy link
Member Author
GromNaN commented Mar 28, 2022

@codedmonkey you worked on this feature; could you review?

@nicolas-grekas
Copy link
Member
nicolas-grekas commented Mar 28, 2022 •
edited
Loading

The default TZ is already configured on the system.

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?

@nicolas-grekas
Copy link
Member

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;
+            }
         }

@GromNaN
Copy link
Member Author
GromNaN commented Mar 28, 2022

Yes, that makes sense and solves the main issue.

@GromNaN GromNaN mentioned this pull request Mar 28, 2022
Merged
@GromNaN
Copy link
Member Author
GromNaN commented Mar 28, 2022

Implemented in a new PR: #45874

@GromNaN GromNaN closed this Mar 28, 2022
fabpot added a commit that referenced this pull request Mar 29, 2022
@fabpot
bug #45874 [HttpKernel]  Resolve DateTime values with default timezon…
1db1443 
…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
@GromNaN GromNaN deleted the param-converter-tz branch March 29, 2022 10:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas left review comments

Assignees
No one assigned
Labels
Feature HttpKernel Status: Needs Review
Projects
None yet
Milestone
6.1
Development

Successfully merging this pull request may close these issues.
3 participants
@GromNaN @nicolas-grekas @carsonbot
0