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

[Validator] allow brackets in the optional query string #30606

Merged
merged 1 commit into from Mar 27, 2019

Conversation

Projects
None yet
8 participants
@eborges78
Copy link

commented Mar 19, 2019

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #30603
License MIT
Doc PR symfony/symfony-docs#...

Add the allowBrackets option from the Url constraint to allow brackets in the optional query string.

@stof

This comment has been minimized.

Copy link
Member

commented Mar 19, 2019

The name allowExtraChars does not seem clear to me if it is actually about allowing brackets. When seeing it, I would have no idea what the extra chars are.

@eborges78 eborges78 closed this Mar 19, 2019

@eborges78 eborges78 reopened this Mar 19, 2019

@nicolas-grekas nicolas-grekas added this to the next milestone Mar 19, 2019

@eborges78 eborges78 force-pushed the eborges78:feature/url branch from 7096ce4 to 74fcf51 Mar 27, 2019

@fabpot

This comment has been minimized.

Copy link
Member

commented Mar 27, 2019

@eborges78 Can you rebase to get rid of the merge commit?

@eborges78 eborges78 force-pushed the eborges78:feature/url branch from 686b6b6 to c76f460 Mar 27, 2019

@eborges78 eborges78 requested review from dunglas, lyrixx and sroze as code owners Mar 27, 2019

@eborges78 eborges78 force-pushed the eborges78:feature/url branch 3 times, most recently from 2940c3d to 4fc4015 Mar 27, 2019

@eborges78

This comment has been minimized.

Copy link
Author

commented Mar 27, 2019

@eborges78 Can you rebase to get rid of the merge commit?

Fixed.

@eborges78 eborges78 force-pushed the eborges78:feature/url branch 3 times, most recently from 3bc4d1a to 5fe210c Mar 27, 2019

@fabpot

This comment has been minimized.

Copy link
Member

commented Mar 27, 2019

Do we really need an option for that? Why would you not allow it?

@eborges78

This comment has been minimized.

Copy link
Author

commented Mar 27, 2019

Do we really need an option for that? Why would you not allow it?

I did not want to alter the existing behavior of the UrlValidator.
Do you think this option can be removed?

@fabpot

This comment has been minimized.

Copy link
Member

commented Mar 27, 2019

I would vote for having this by default with no options. WDYT @symfony/deciders?

@javiereguiluz

This comment has been minimized.

Copy link
Member

commented Mar 27, 2019

I agree with Fabien. According to RFC 3986 the following line is the regular expression for breaking-down a well-formed URI reference into its components:

^(([^:/?#]+):)?(//([^/?#]*))?([^?#]*)(\?([^#]*))?(#(.*))?

This is the query string part -> (\?([^#]*))? So nothing prevents using [ and ] and, therefore, we shouldn't treat them as a special thing.

@eborges78

This comment has been minimized.

Copy link
Author

commented Mar 27, 2019

@fabpot @javiereguiluz Thank you for your feedback. so I removed the option and allowed brackets by default

@fabpot

fabpot approved these changes Mar 27, 2019

@fabpot fabpot force-pushed the eborges78:feature/url branch from 0ed1d4c to 40dc4c8 Mar 27, 2019

@fabpot

This comment has been minimized.

Copy link
Member

commented Mar 27, 2019

Thank you @eborges78.

@fabpot fabpot merged commit 40dc4c8 into symfony:master Mar 27, 2019

1 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Mar 27, 2019

feature #30606 [Validator] allow brackets in the optional query strin…
…g (Emmanuel BORGES)

This PR was squashed before being merged into the 4.3-dev branch (closes #30606).

Discussion
----------

[Validator] allow brackets in the optional query string

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #30603
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!-- required for new features -->

Add the `allowBrackets` option from the Url constraint to allow brackets in the optional query string.

Commits
-------

40dc4c8 [Validator] allow brackets in the optional query string

@nicolas-grekas nicolas-grekas modified the milestones: next, 4.3 Apr 30, 2019

@fabpot fabpot referenced this pull request May 9, 2019

Merged

Release v4.3.0-BETA1 #31435

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.