Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Closed

Conversation

@teohhanhui
Copy link
Contributor

QA
Branch?3.1
Bug fix?no
New feature?no
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed ticketsN/A
LicenseMIT
Doc PRN/A

@teohhanhuiteohhanhui changed the titleDon't overwrite $format variable in DateTimeNormalizer::normalize[Serializer] Don't overwrite $format variable in DateTimeNormalizer::normalizeOct 14, 2016
@dunglas
Copy link
Member

👍

1 similar comment
@xabbuh
Copy link
Member

👍

@fabpot
Copy link
Member

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
ContributorAuthor

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

@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
ContributorAuthor

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:

@teohhanhuihttps://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 reacted with thumbs up emoji

@teohhanhui
Copy link
ContributorAuthor

Should I close this?

nicolas-grekas reacted with thumbs up emoji

@fabpotfabpot closed thisOct 17, 2016
@teohhanhuiteohhanhui deleted the datetimedenormalizer-nit branchOctober 18, 2016 04:15
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@teohhanhui@dunglas@xabbuh@fabpot@nicolas-grekas@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp