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

Decode attributes pulled from URI.parse #7593

Merged
merged 1 commit into from
Oct 26, 2012
Merged

Conversation

veader
Copy link
Contributor

@veader veader commented Sep 10, 2012

The RFC indicates that username and passwords may be encoded.
http://tools.ietf.org/html/rfc2396#section-3.2.2

Found this trying to use the mysql://username:password@host:port/db and having special characters in the password which needed to be URI encoded.

@al2o3cr
Copy link
Contributor

al2o3cr commented Sep 10, 2012

A test case would be helpful - probably best to add it in test/cases/connection_specification/resolver_test.rb.

@veader
Copy link
Contributor Author

veader commented Sep 11, 2012

Tests included. Also silenced some 1.9.x deprecation warning for URI.unescape

@frodsan
Copy link
Contributor

frodsan commented Oct 26, 2012

Could you squash your commits? Thanks.

@veader
Copy link
Contributor Author

veader commented Oct 26, 2012

@frodsan done. looks like i botched my commit message in the squash. :/

@frodsan
Copy link
Contributor

frodsan commented Oct 26, 2012

/cc @jonleighton @tenderlove

@@ -73,6 +73,8 @@ def connection_url_to_hash(url) # :nodoc:
:database => config.path.sub(%r{^/},""),
:host => config.host }
spec.reject!{ |_,value| value.blank? }
uri_parser = URI.const_defined?(:Parser) ? URI::Parser.new : URI
Copy link
Member

Choose a reason for hiding this comment

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

When the Parser is not available?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was trying to make this safe for 1.8. Not sure it's needed at this point. Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

You don't have to worry about 1.8. Master branch version only supports >= 1.9.3.

@rafaelfranca
Copy link
Member

Could you add a CHANGELOG entry?

@veader
Copy link
Contributor Author

veader commented Oct 26, 2012

@rafaelfranca sure, do you want me to incorporate the changes above as well?

@veader
Copy link
Contributor Author

veader commented Oct 26, 2012

@rafaelfranca Done. Squashed with previous and incorporated comments from above review. Thanks!

@@ -36,9 +36,13 @@
implementation is very small and it's probably not worth putting users
through lots of annoying deprecation warnings.

*Shawn Veader*
Copy link
Member

Choose a reason for hiding this comment

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

This CHANGELOG entry is wrong. It should be at top of the file and the author name is below the change.

The RFC indicates that username and passwords may be encoded.
http://tools.ietf.org/html/rfc2396#section-3.2.2

Found this trying to use the mysql://username:password@host:port/db and having special characters in the password which needed to be URI encoded.
@veader
Copy link
Contributor Author

veader commented Oct 26, 2012

@rafaelfranca Apologies... fixed. (Someone needs sleep and/or more coffee.)

@rafaelfranca
Copy link
Member

Thank you so much for the pull request.

rafaelfranca added a commit that referenced this pull request Oct 26, 2012
Decode attributes pulled from URI.parse
@rafaelfranca rafaelfranca merged commit 35f2a09 into rails:master Oct 26, 2012
@frodsan
Copy link
Contributor

frodsan commented Oct 26, 2012

👍 Thanks.

rafaelfranca added a commit that referenced this pull request Oct 29, 2012
Decode attributes pulled from URI.parse
Conflicts:
	activerecord/CHANGELOG.md
	activerecord/lib/active_record/connection_adapters/connection_specification.rb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants