Skip to content
This repository has been archived by the owner on Jul 7, 2022. It is now read-only.

Fixes #10 - URL decoding #15

Merged
merged 4 commits into from Mar 26, 2015
Merged

Fixes #10 - URL decoding #15

merged 4 commits into from Mar 26, 2015

Conversation

ollietreend
Copy link
Contributor

Fix for issue #10 - Add url decoding to dsn parts.

URL host, user and pass parameters are now encoded and decoded.

@ollietreend ollietreend changed the title Issue #10 - URL decoding Fix #10 - URL decoding Mar 9, 2015
@ollietreend ollietreend changed the title Fix #10 - URL decoding Fixes #10 - URL decoding Mar 9, 2015
@ollietreend
Copy link
Contributor Author

phpcs is failing on a line that I didn't actually change. It also fails on the master branch when I run it locally, but the Travis build is still passing on the github master for some reason - I have no idea why.

@ollietreend
Copy link
Contributor Author

@AD7six is there any chance I can get some feedback on this please? I'm keen for this to be merged in as I want to start using SSL connections for my SMTP gateway config :)

@coveralls
Copy link

Coverage Status

Coverage increased (+0.86%) to 77.69% when pulling f71b8f7 on ollietreend:issue-10 into 3f33829 on AD7six:master.

@@ -239,6 +239,18 @@ protected function parseUrlArray($url)

$url = array_merge($this->uriKeys, $url);

if (isset($url['host'])) {
Copy link
Owner

Choose a reason for hiding this comment

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

Can you chnge this pleas to be

foreach['host', 'user', 'pass'] as $key) {
    if (!isset($url[$key]) {
        continue;
    }
    $url[$key] = urldecode($url[$key]);
}

please - I.e. make is so that adding another key (should that be necessary in the future) is just adding to an array rather than copy and paste.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. I didn't want to over-complicate things with a loop, but it will certainly keep things DRYer. I'll change them to foreach.

@AD7six
Copy link
Owner

AD7six commented Mar 26, 2015

I missed the notification for this - sorry for the delay.

I'm unclear which keys need decoding - but if it doesn't break any tests, I'm assuming it won't break any usage for the moment.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.49%) to 77.31% when pulling 8f7a4e2 on ollietreend:issue-10 into 3f33829 on AD7six:master.

AD7six added a commit that referenced this pull request Mar 26, 2015
@AD7six AD7six merged commit 69ff74a into AD7six:master Mar 26, 2015
@AD7six
Copy link
Owner

AD7six commented Mar 26, 2015

Thanks =)

@ollietreend
Copy link
Contributor Author

Thanks for merging @AD7six. Could we have a new version tagged so these changes can be pulled in with composer please?

@AD7six
Copy link
Owner

AD7six commented Mar 28, 2015

0.4.0 released =)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants