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] fail reverse transforming invalid RFC 3339 dates#28466

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:2.8fromxabbuh:issue-28455
Sep 22, 2018

Conversation

@xabbuh
Copy link
Member

QA
Branch?2.8
Bug fix?yes
New feature?no
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets#28455
LicenseMIT
Doc PR

return;
}

if (!preg_match('/^(\d{4})-(\d{2})-(\d{2})T\d{2}:\d{2}(?::\d{2})?(?:\.\d)?(?:Z|(?:(?:\+|-)\d{2}:\d{2}))$/',$rfc3339,$matches)) {

Choose a reason for hiding this comment

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

Is there a possibility this stricter regexp could be a BC break? What would be the downside of keeping the previous regexp?

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

In theory, if someone doesn't use their browser but submits dates manually that could indeed feel like a BC break to them. But since#28372 this transformer isn't used anymore by any of the built-in form types. So yes, maybe we should relax this pattern here and inDateTimeToHtml5LocalDateTimeTransformer a bit. The question then is, how much relaxation should we do and when is a failure acceptable?

Copy link
Member

@nicolas-grekasnicolas-grekasSep 19, 2018
edited
Loading

Choose a reason for hiding this comment

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

Just keeping the previous one?'/(\d{4})-(\d{2})-(\d{2})/'? (should be also applied toDateTimeToHtml5LocalDateTimeTransformer) - or at least put the added trailing patterns in a(?:...)? to make them optional?

Copy link
Member

@javiereguiluzjaviereguiluzSep 21, 2018
edited
Loading

Choose a reason for hiding this comment

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

I don't understand this. If we're implementing RFC 339, we can't discuss about the regexp, right? We should use the one defined in that RFC. Since we're fixing a bug, BC breaks are not considered. Strictly speaking, all bug fixes are BC breaks because we're changing the previous behaviour.

HeahDude reacted with thumbs up emoji
Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Javier has a valid point here. Let's just keep it the way it is.

Copy link
Member

Choose a reason for hiding this comment

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

I've not checked, but is usage like#28703 supposed to work? I mean, it worked before... by accident, right? Not saying we should not fix the BC break of course.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Using relative date formats was never supposed to work. It used to work accidentally like many invalid dates too. If we are really to make that supported, that will means that we have to abstain completely from detecting invalid input.

Copy link
Member

@nicolas-grekasnicolas-grekasOct 3, 2018
edited
Loading

Choose a reason for hiding this comment

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

We were accepting any parseable date, with the format allowed by the date parser of PHP. That's a working validation strategy to me also.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

The date parser accepts way too much input (see#28455 for such an example). I don't see how that is better.

Choose a reason for hiding this comment

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

I won't argue about how being stricter can be better (or not, e.g. for accessibility) - but this is still a regression.

@nicolas-grekas
Copy link
Member

Thank you@xabbuh.

@nicolas-grekasnicolas-grekas merged commitee4ce43 intosymfony:2.8Sep 22, 2018
nicolas-grekas added a commit that referenced this pull requestSep 22, 2018
…abbuh)This PR was merged into the 2.8 branch.Discussion----------[Form] fail reverse transforming invalid RFC 3339 dates| Q             | A| ------------- | ---| Branch?       | 2.8| Bug fix?      | yes| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets |#28455| License       | MIT| Doc PR        |Commits-------ee4ce43 fail reverse transforming invalid RFC 3339 dates
@xabbuhxabbuh deleted the issue-28455 branchSeptember 22, 2018 20:29
This was referencedSep 30, 2018
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@fabpotfabpotfabpot left review comments

@javiereguiluzjaviereguiluzjaviereguiluz left review comments

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

+1 more reviewer

@HeahDudeHeahDudeHeahDude approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

2.8

Development

Successfully merging this pull request may close these issues.

6 participants

@xabbuh@nicolas-grekas@fabpot@javiereguiluz@HeahDude@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp