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

[Form] fail reverse transforming invalid RFC 3339 dates #28466

Merged
merged 1 commit into from Sep 22, 2018

Conversation

Projects
None yet
6 participants
@xabbuh
Copy link
Member

commented Sep 14, 2018

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
@@ -69,6 +69,10 @@ public function reverseTransform($rfc3339)
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)) {

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Sep 18, 2018

Member

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

This comment has been minimized.

Copy link
@xabbuh

xabbuh Sep 19, 2018

Author Member

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 in DateTimeToHtml5LocalDateTimeTransformer a bit. The question then is, how much relaxation should we do and when is a failure acceptable?

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Sep 19, 2018

Member

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

This comment has been minimized.

Copy link
@javiereguiluz

javiereguiluz Sep 21, 2018

Member

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.

This comment has been minimized.

Copy link
@xabbuh

xabbuh Sep 21, 2018

Author Member

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

This comment has been minimized.

Copy link
@fabpot

fabpot Oct 3, 2018

Member

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.

This comment has been minimized.

Copy link
@xabbuh

xabbuh Oct 3, 2018

Author Member

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.

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Oct 3, 2018

Member

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.

This comment has been minimized.

Copy link
@xabbuh

xabbuh Oct 3, 2018

Author Member

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

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Oct 3, 2018

Member

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

This comment has been minimized.

Copy link
Member

commented Sep 22, 2018

Thank you @xabbuh.

@nicolas-grekas nicolas-grekas merged commit ee4ce43 into symfony:2.8 Sep 22, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

nicolas-grekas added a commit that referenced this pull request Sep 22, 2018

bug #28466 [Form] fail reverse transforming invalid RFC 3339 dates (x…
…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

@xabbuh xabbuh deleted the xabbuh:issue-28455 branch Sep 22, 2018

This was referenced Sep 30, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.