Skip to content

[Serializer] Don't overwrite $format variable in DateTimeNormalizer::normalize #20216

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

Closed
wants to merge 1 commit into from

Conversation

teohhanhui
Copy link
Contributor

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

@teohhanhui teohhanhui changed the title Don't overwrite $format variable in DateTimeNormalizer::normalize [Serializer] Don't overwrite $format variable in DateTimeNormalizer::normalize Oct 14, 2016
@teohhanhui teohhanhui force-pushed the datetimedenormalizer-nit branch from 317a06e to 8bd5b0a Compare October 14, 2016 04:39
@dunglas
Copy link
Member

dunglas commented Oct 14, 2016

👍

1 similar comment
@xabbuh
Copy link
Member

xabbuh commented Oct 14, 2016

👍

@fabpot
Copy link
Member

fabpot commented Oct 14, 2016

Does it fix a bug? I fail to see how this is important to change; we can probably find such other examples in the codebase.

@teohhanhui
Copy link
Contributor Author

What is the policy? I can resubmit this for master if that's better. Or I
can make it part of #20217 if that's fine.

On Fri, 14 Oct 2016, 22:26 Fabien Potencier, notifications@github.com
wrote:

Does it fix a bug? I fail to see how this is important to change; we can
probably find such other examples in the codebase.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#20216 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAhf67nUMfnONy4L8_bvPKVrs8Sg8I-0ks5qz5EugaJpZM4KWoXQ
.

@fabpot
Copy link
Member

fabpot commented Oct 14, 2016

@teohhanhui I'm not against this change, but this adds a lot of overhead for the core team for no actual benefit (reviewing the change to be sure it's safe, merging the PR, possible conflicts when merging old branches to newer ones, ...). So, I prefer to avoid PRs that don't really solve any issue. Doing this as part of #20217 is indeed an option, but honestly, I don't see the need for this change.

@teohhanhui
Copy link
Contributor Author

Sometimes it's just hard to resist the urge of wanting to fix minor code
quality issues when you accidentally see it when working on something else.

So, just for the record, should minor code quality improvement PRs like
this generally be avoided? If that's the team's decision I shall of course
abide by it.

On Fri, 14 Oct 2016, 23:13 Fabien Potencier, notifications@github.com
wrote:

@teohhanhui https://github.com/teohhanhui I'm not against this change,
but this adds a lot of overhead for the core team for no actual benefit
(reviewing the change to be sure it's safe, merging the PR, possible
conflicts when merging old branches to newer ones, ...). So, I prefer to
avoid PRs that don't really solve any issue. Doing this as part of #20217
#20217 is indeed an option, but
honestly, I don't see the need for this change.


You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
#20216 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAhf6xgl4iXJBuPrrBvo0L6RHUbZlFbyks5qz5wugaJpZM4KWoXQ
.

@nicolas-grekas
Copy link
Member

The issue is that we have some experience in breaking Symfony by merging apparently minor things... So we learned to be careful...

@teohhanhui
Copy link
Contributor Author

Should I close this?

@fabpot fabpot closed this Oct 17, 2016
@teohhanhui teohhanhui deleted the datetimedenormalizer-nit branch October 18, 2016 04:15
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.

6 participants