8000 [Form] DateTimeImmutable norm data in DateTime form types by vudaltsov · Pull Request #25273 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Form] DateTimeImmutable norm data in DateTime form types #25273

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 4 commits into from
Closed

[Form] DateTimeImmutable norm data in DateTime form types #25273

wants to merge 4 commits into from

Conversation

vudaltsov
Copy link
Contributor
Q A
Branch? master
Bug fix? no
New feature? no
BC breaks? no
Deprecations? yes
Tests pass? yes
Fixed tickets
License MIT
Doc PR symfony/symfony-docs#...

This PR suggests a way to use \DateTimeImmutable objects for norm data in DateTimeType, DateType and TimeType.

A complete BC layer for transformers is provided. All protected calls are also taken into account.

If this PR gets merged, we can then easily add input=datetimeimmutable with no model transformers to support \DateTimeImmutable model data.

@vudaltsov
Copy link
Contributor Author

ping @xabbuh , @stof, @HeahDude

@vudaltsov vudaltsov mentioned this pull request Dec 2, 2017
Closed
Simperfit
Simperfit suggested changes Dec 3, 2017
View reviewed changes
Copy link
Contributor
@Simperfit Simperfit left a comment

Choose a reason for hiding this comment

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

You need to add tests for all the new classes you created.

...ymfony/Component/Form/Extension/Core/DataTransformer/DateTimeImmutableToArrayTransformer.php Outdated
class DateTimeImmutableToArrayTransformer extends BaseDateTimeTransformer
{
private $pad;

Copy link
Contributor

Choose a reason for hiding this comment

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

no need for this empty line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanx, done

...ponent/Form/Extension/Core/DataTransformer/DateTimeImmutableToLocalizedStringTransformer.php Outdated
class DateTimeImmutableToLocalizedStringTransformer extends BaseDateTimeTransformer
{
private $dateFormat;

Copy link
Contributor

Choose a reason for hiding this comment

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

same

@vudaltsov
Copy link
Contributor Author

Added tests.

@nicolas-grekas nicolas-grekas added this to the 4.1 milestone Dec 4, 2017
@stof
Copy link
Member
stof commented Dec 4, 2017

I'm not sure about the BC impact of changing the type of the norm data. I suggest adding the new input type first, without changing the norm data.

@vudaltsov
Copy link
Contributor Author

Here @HeahDude posted:

If a new major version of Symfony bumps the requirement to PHP 5.5+, the normalized data could then be switched to the DateTimeImmutable class to simplify the logic (which is currentl 8000 y cloning the DateTime to avoid modifying the original one.

That's what I tried to do in my PR. My change is pretty straightforward: I copied and renamed transformers and their tests and removed clone.

If we really want to make norm data immutable than I think we should do it first. If we don't - than yes, just add input=datetimimmutable and a corresponding transformer.

But with what I suggest immutable dates will be supported in the first place. Only DateTimes will be converted. In this way immutable norm data helps to follow the trend of replacing DateTime with DateTimeImmutable.

@stof
Copy link
Member
stof commented Dec 4, 2017

you need to add the new allowed value for input option. Otherwise, immutable datetime are not supported (they are used as norm data, but cannot be used as model data, which is the requested feature)

@vudaltsov
Copy link
Contributor Author

Sure! I planned to do that in a separate PR. Because it's gonna have a couple of new tests.
In this PR I just provide a ground for that by changing the norm data.

If you wish, I can add commits right here.

@vudaltsov
Copy link
Contributor Author

@Simperfit, could you please review my tests?

Simperfit
Simperfit reviewed Dec 13, 2017
View reviewed changes
Copy link
Contributor
@Simperfit Simperfit left a comment

Choose a reason for hiding this comment

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

The tests looks good to me,

...ponent/Form/Extension/Core/DataTransformer/DateTimeImmutableToLocalizedStringTransformer.php
/**
* @see BaseDateTimeTransformer::formats for available format options
*
* @param string $inputTimezone The name of the input timezone
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think you need all of this.

...nt/Form/Tests/Extension/Core/DataTransformer/DateTimeImmutableToTimestampTransformerTest.php
{
$reverseTransformer = new DateTimeImmutableToTimestampTransformer();

$this->{method_exists($this, $_ = 'expectException') ? $_ : 'setExpectedException'}('Symfony\Component\Form\Exception\TransformationFailedException');
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe you should add a new var for this.

@vudaltsov vudaltsov mentioned this pull request Dec 22, 2017
Closed
@vudaltsov
Copy link
Contributor Author
vudaltsov commented Dec 22, 2017 •
edited
Loading

Closed in favor of #25582.

@vudaltsov vudaltsov closed this Dec 22, 2017
@vudaltsov vudaltsov deleted the form-date-immutable branch January 23, 2018 23:46
fabpot added a commit that referenced this pull request Feb 7, 2018
@fabpot
feature #25582 [Form] Support \DateTimeImmutable (vudaltsov)
ec89ac8 
This PR was squashed before being merged into the 4.1-dev branch (closes #25582).

Discussion
----------

[Form] Support \DateTimeImmutable

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #9508
| License       | MIT
| Doc PR        | symfony/symfony-docs#8920

This PR implements `input=datetime_immutable`. Replaces #25273.

Commits
-------

034f8b2 [Form] Support \DateTimeImmutable
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Simperfit Simperfit Simperfit requested changes

Assignees
No one assigned
Labels
Deprecation Form Status: Needs Work
Projects
None yet
Milestone
4.1
Development

Successfully merging this pull request may close these issues.
5 participants
@vudaltsov @stof @Simperfit @nicolas-grekas @carsonbot
0