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

Fix parsing schemeless connection URLs #2287

Closed
wants to merge 3 commits into from

Conversation

deeky666
Copy link
Member

@deeky666 deeky666 commented Jan 9, 2016

Taken over from and inspired by #863. Fixes #1183.

Also includes proper handling of the default driver connection parameters pdo, driver and driverClass when used together with connection URLs.
Throws exception now if URL is given but neither URL scheme nor driver parameter nor driverClass parameter is given. If also given a default PDO instance, DBAL would currently use that one instead and silently discard connection URL information.

Finally I have done a minor refactoring of the connection URL parsing by simplifying the logic and splitting into smaller private methods. It is much more comprehensible now IMO. Also added a lot of comments to keep track of all the weird things that happen there :D

…river connection parameters

- includes extended default driver connection parameter handling
- minor refactoring

fixes doctrine#1183
@deeky666
Copy link
Member Author

deeky666 commented Jan 9, 2016

Fixed mocking zeh PDO

$url
)
);
}

Choose a reason for hiding this comment

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

I'd put this into a separate static method. To me only looking at this code it is not clear, that when $url is given here, it misses one special part. Maybe missingSchemeRequiredDriver() or something like that?

Copy link
Member Author

Choose a reason for hiding this comment

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

Also no strong opinion here. @Ocramius @zeroedin-bill thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

Would have done a separate method as well, yep.

Copy link
Member

Choose a reason for hiding this comment

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

Errata: seen now that 'url' may be passed in from multiple contexts. It is indeed better to let the exception class decide here.

@kingcrunch
Copy link

Since this was my approach originally I left some comments :) Still somebody with a better feeling for the big picture should have a look.

Ocramius added a commit that referenced this pull request Sep 9, 2016
Ocramius added a commit that referenced this pull request Sep 9, 2016
Ocramius added a commit that referenced this pull request Sep 9, 2016
@Ocramius Ocramius closed this in 4a85a58 Sep 9, 2016
@Ocramius
Copy link
Member

Ocramius commented Sep 9, 2016

Merged, thanks @deeky666 and @kingcrunch!

master: 4a85a58
2.5: dcce6f8

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

Successfully merging this pull request may close these issues.

4 participants