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

URL-decode URL-style DSN #2504

Merged
merged 2 commits into from Jan 15, 2017
Merged

URL-decode URL-style DSN #2504

merged 2 commits into from Jan 15, 2017

Conversation

@c960657
Copy link
Contributor

c960657 commented Sep 12, 2016

The URL-style DSN does not support escaping, so if the username/password/database name contains certain characters, the connection options simply cannot be represented this way.

This patch URL-decodes the different parts. It introduces a small BC break for usernames/passwords/database names containing the character %.

@deeky666

This comment has been minimized.

Copy link
Member

deeky666 commented Jan 14, 2017

@Ocramius this looks like a reasonable improvement. The BC question arises of course, especially significant for passwords IMO. What do you think? Shall we accept the slight BC and target this patch for 2.6 only? Also I think this needs documentation.

@Ocramius

This comment has been minimized.

Copy link
Member

Ocramius commented Jan 14, 2017

This is sane, and can be landed in 2.6, but it needs documenting in UPGRADE.md before it can be merged.

@c960657 can you add notes for how an upgrade would be performed, and what the BC break for % characters would be? I'd like to also have the % character as part of the test suite data-providers (additional entry to what you added), so that the expected output can be viewed by users hitting the problem, and further regressions prevented.

@Ocramius Ocramius added this to the 2.6 milestone Jan 14, 2017
@Ocramius Ocramius self-requested a review Jan 14, 2017
Copy link
Member

Ocramius left a comment

Requesting changes as per #2504 (comment)

@deeky666

This comment has been minimized.

Copy link
Member

deeky666 commented Jan 14, 2017

@c960657 also an entry about URL econding/deconding should make its way into the DBAL documentation.

@c960657 c960657 force-pushed the c960657:urldecode-dsn branch from d788294 to 7bc59d4 Jan 15, 2017
@c960657 c960657 force-pushed the c960657:urldecode-dsn branch from 7bc59d4 to cdeb9e8 Jan 15, 2017
@c960657

This comment has been minimized.

Copy link
Contributor Author

c960657 commented Jan 15, 2017

I have added a test and some upgrade information.

@Ocramius Ocramius self-assigned this Jan 15, 2017
@Ocramius

This comment has been minimized.

Copy link
Member

Ocramius commented Jan 15, 2017

@c960657 awesome, thanks!

@Ocramius Ocramius merged commit bfecbeb into doctrine:master Jan 15, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@c960657 c960657 deleted the c960657:urldecode-dsn branch Jan 16, 2017
@Ocramius Ocramius removed the Missing Tests label Jul 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.