Skip to content
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

Remove value conversion to DateTime in ConstraintValidator #34961

Closed
wants to merge 1 commit into from

Conversation

norkunas
Copy link
Contributor

Q A
Branch? master
Bug fix? no
New feature? no
Deprecations? no
Tickets -
License MIT
Doc PR -

IntlDateFormatter supports formatting DateTimeImmutable instances since php 7.1.5, so we don't need anymore to convert it to DateTime instance when formatting values in ConstraintValidator.
https://3v4l.org/jW5CM

@fancyweb
Copy link
Contributor

fancyweb commented Dec 13, 2019

That's true and symfony/intl halso supports it now (see #32947). At the moment this change kind of conflict with #34904. However, that means I can use \DateTimeImmutable there.

@norkunas
Copy link
Contributor Author

That's true and symfony/intl halso supports it now (see #32947). At the moment this change kind of conflict with #34904. However, that means I can use \DateTimeImmutable there.

Sorry, missed this :)

@fancyweb
Copy link
Contributor

Keep this one open for now.

However, that means I can use \DateTimeImmutable there

Actually not sure it worth it to check the PHP version and symfony/intl for that...

@fancyweb
Copy link
Contributor

I think this PR can be closed because #34904 was merged. A new \DateTime instance is now formatted. Using a \DateTimeImmutable instead would not change anything.

@norkunas
Copy link
Contributor Author

But what's the point of reconstructing another instance?

@fancyweb
Copy link
Contributor

To force the formatted \DateTime timezone to UTC.

@norkunas
Copy link
Contributor Author

Ok 👍

@norkunas norkunas closed this Dec 19, 2019
@norkunas norkunas deleted the skip-conversion branch December 19, 2019 12:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants