Skip to content

[FrameworkBundle] Add the clock.default_timezone option #58434

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
alexandre-daubois wants to merge 1 commit into from
Closed

[FrameworkBundle] Add the clock.default_timezone option #58434

alexandre-daubois wants to merge 1 commit into from

Conversation

@alexandre-daubois
Copy link
Member

@alexandre-daubois alexandre-daubois commented Oct 2, 2024 •
edited
Loading

Q A
Branch? 7.2
Bug fix? no
New feature? yes
Deprecations? no
Issues -
License MIT

We are introducing the clock component in a project. Because of legacy reasons, dates are stored using the Europe/Paris timezone instead of using UTC. Because we'd like to use the clock service and the ClockAwareTrait, it would be nice to configure the timezone to use right in the framework configuration, under a new option called clock.default_timezone:

framework:
    clock:
        default_timezone: Europe/Paris

This adds more flexibility to developers to configure the default timezone to use. Even if the clock gets a default timezone using date_default_timezone_get(), having the possibility to set this option would remove some magic. More than that, it makes the clock service consistent across different PHP installation that may have any value in their php.ini.

@carsonbot carsonbot added this to the 7.2 milestone Oct 2, 2024
stof
stof reviewed Oct 2, 2024
View reviewed changes
nicolas-grekas
nicolas-grekas reviewed Oct 3, 2024
View reviewed changes
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment •
edited
Loading

Choose a reason for hiding this comment

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

To challenge this: not setting the ini option while setting the config option might create discrepancies between parts that use the time without getting through the clock component. Not sure this is a win. Having better control of ini settings might be preferable. When using Platform.sh for example, you can commit a php.ini file at the root of your app. That's a better alternative to me.

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php
}

try {
return new \DateTimeZone($v);
Copy link
Member

@nicolas-grekas nicolas-grekas Oct 3, 2024

Choose a reason for hiding this comment

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

this validation logic prevents using env vars for configuring the option

@javiereguiluz javiereguiluz added the ❄️ Feature Freeze Important Pull Requests to finish before the next Symfony "feature freeze" label Oct 3, 2024
@alexandre-daubois
Copy link
Member Author

alexandre-daubois commented Oct 4, 2024

I understand, maybe this option could come with a warning in the ->info() part about it? My use case is that we have several (legacy or not) project running with the same php.ini in our team. This option would allow to configure a timezone at the framework level only in the dev env, for example. This way, we don't have to touch the php.ini file that works great with the other projects when running multiple projects on the same machine.

However, I understand your point of view about omitting the option in php.ini. This could be indicated in the explanation of the option?

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Oct 4, 2024

I'd say you need to improve your dev stack: eg if you use symfony serve, php.ini in the root of the app is also loaded. Just use it :)

@alexandre-daubois
Copy link
Member Author

alexandre-daubois commented Oct 4, 2024

I wasn't aware of this feature. Indeed, that's way better. Let's close then, thanks!

@alexandre-daubois
Copy link
Member Author

alexandre-daubois commented Oct 4, 2024

Thanks for this feedback @ro0NL, would this PR solve your issue? Could you maybe elaborate a bit on how this would help you if possible? I'd be happy to reopen if relevant 🙂

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

@stof stof stof left review comments

@OskarStark OskarStark OskarStark approved these changes

Assignees

No one assigned

Labels

Feature FrameworkBundle ❄️ Feature Freeze Important Pull Requests to finish before the next Symfony "feature freeze" Status: Reviewed

Projects

None yet

Milestone

7.2

Development

Successfully merging this pull request may close these issues.

6 participants

@alexandre-daubois @nicolas-grekas @stof @OskarStark @javiereguiluz @carsonbot