Skip to content

[Form][DX] derive default timezone from reference_date option when possible #32762

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

Merged
merged 1 commit into from
Jul 29, 2019

Conversation

yceruto
Copy link
Member

@yceruto yceruto commented Jul 26, 2019

Q A
Branch? 4.4
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR -

Currently, when reference_date and model_timezone are provided but are not the same, we get a validation exception:

if (null !== $options['reference_date'] && $options['reference_date']->getTimezone()->getName() !== $options['model_timezone']) {
throw new InvalidConfigurationException(sprintf('The configured "model_timezone" (%s) must match the timezone of the "reference_date" (%s).', $options['model_timezone'], $options['reference_date']->getTimezone()->getName()));
}

But that happen even if the model_timezone is null !

I propose to relax this behavior by passing the time zone of the configured reference_date to this model_timezone (by default), thus avoiding annoyances.

@yceruto yceruto added this to the next milestone Jul 26, 2019
@carsonbot carsonbot added Status: Needs Review Form DX DX = Developer eXperience (anything that improves the experience of using Symfony) Feature labels Jul 26, 2019
@xabbuh
Copy link
Member

xabbuh commented Jul 26, 2019

This looks good. Can you also add a test for this?

@yceruto
Copy link
Member Author

yceruto commented Jul 28, 2019

Test added.

@xabbuh
Copy link
Member

xabbuh commented Jul 29, 2019

Thank you @yceruto.

@xabbuh xabbuh merged commit a62feea into symfony:4.4 Jul 29, 2019
xabbuh added a commit that referenced this pull request Jul 29, 2019
… option when possible (yceruto)

This PR was merged into the 4.4 branch.

Discussion
----------

[Form][DX] derive default timezone from reference_date option when possible

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

Currently, when `reference_date` and `model_timezone` are provided but are not the same, we get a validation exception:
https://github.com/symfony/symfony/blob/9216cb75ac11db014262f2d7fe5cb77b0b54db59/src/Symfony/Component/Form/Extension/Core/Type/TimeType.php#L48-L50

But that happen even if the `model_timezone` is `null` !

I propose to relax this behavior by passing the time zone of the configured `reference_date` to this `model_timezone` (by default), thus avoiding annoyances.

Commits
-------

a62feea [DX] derive default timezone from reference_date option when possible
@yceruto yceruto deleted the time_type branch July 29, 2019 15:54
@nicolas-grekas nicolas-grekas modified the milestones: next, 4.4 Oct 27, 2019
This was referenced Nov 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DX DX = Developer eXperience (anything that improves the experience of using Symfony) Feature Form Status: Reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants