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

[Form] Use !isset for checks cause this doesn't falsely include 0#41000

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

Merged
nicolas-grekas merged 1 commit intosymfony:4.4fromKDederichs:fix/40999
May 27, 2021

Conversation

@KDederichs
Copy link
Contributor

QA
Branch?5.2
Bug fix?yes
New feature?no
Deprecations?no
TicketsFix#40999
LicenseMIT

By using isset 0 won't falsely included anymore

@carsonbotcarsonbot added this to the5.2 milestoneApr 30, 2021
@carsonbotcarsonbot changed the titleUse !isset for checks cause this doesn't falsely include 0[Form] Use !isset for checks cause this doesn't falsely include 0Apr 30, 2021
@derrabus
Copy link
Member

Can you add a test for this issue?

Question:empty() also filters out empty strings while!isset() doesn't. Is this relevant here?

@Nyholm
Copy link
Member

Excellent PR. Thank you.

If you add a small test for this I'll be happy to merge.

@KDederichs
Copy link
ContributorAuthor

Excellent PR. Thank you.

If you add a small test for this I'll be happy to merge.

I'm kinda waiting on a reply in the mentioned issue tbh.
Cause it's really only a bug in that specific usecase, which I'm not 100% sure should be valid anyways.

@KDederichs
Copy link
ContributorAuthor

Sorry took me a while to get to it but I got some tests that would fail without the changes on both PRs

@xabbuh
Copy link
Member

This applies to4.4 too, right?

@KDederichs
Copy link
ContributorAuthor

Yeah it's actually a thing in all symfony versions I think.

@derrabusderrabus modified the milestones:5.2,4.4May 17, 2021
'%s-%s-%s %s:%s:%s',
empty($value['year']) ?$this->referenceDate->format('Y') :$value['year'],
empty($value['month']) ?$this->referenceDate->format('m') :$value['month'],
empty($value['day']) ?$this->referenceDate->format('d') :$value['day'],

Choose a reason for hiding this comment

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

can you please ensure we have a test case using0 for year+month+day?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Added it as test for DateType.

@nicolas-grekasnicolas-grekas changed the base branch from5.2 to4.4May 27, 2021 12:54
@nicolas-grekas
Copy link
Member

Thank you@KDederichs.

@nicolas-grekasnicolas-grekas merged commit9b89b2e intosymfony:4.4May 27, 2021
This was referencedMay 31, 2021
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas left review comments

@jderussejderussejderusse left review comments

@derrabusderrabusderrabus approved these changes

@xabbuhxabbuhAwaiting requested review from xabbuhxabbuh is a code owner

@ycerutoycerutoAwaiting requested review from ycerutoyceruto is a code owner

@chalasrchalasrAwaiting requested review from chalasr

@dunglasdunglasAwaiting requested review from dunglas

@lyrixxlyrixxAwaiting requested review from lyrixx

@srozesrozeAwaiting requested review from sroze

@wouterjwouterjAwaiting requested review from wouterj

+1 more reviewer

@ostroluckyostroluckyostrolucky left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

4.4

Development

Successfully merging this pull request may close these issues.

8 participants

@KDederichs@derrabus@Nyholm@xabbuh@nicolas-grekas@ostrolucky@jderusse@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp