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.

Sign up for GitHub

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

Jump to bottom

Fix broken IPv6 addresses handling #19788

Merged
merged 1 commit into from
Sep 1, 2015
Merged

Fix broken IPv6 addresses handling #19788

merged 1 commit into from
Sep 1, 2015

Conversation

cmdrclueless
Copy link
Contributor

See Issue #19784 and closed pull #19786 for reference.

@al2o3cr
Copy link
Contributor

al2o3cr commented Apr 18, 2015

Question: why is ActionDispatch::Http::URL splitting out protocol / host / port instead of using the built-in functionality in stdlib's URI? I suspect it might be for performance reasons, but digging through history all the way back to 3.0 didn't show any discussion. Using the stdlib version would have prevented this bug.

rafaelfranca
rafaelfranca reviewed Apr 19, 2015
View reviewed changes
actionpack/test/dispatch/routing/ipv6_redirect_test.rb
@@ -0,0 +1,45 @@
require 'abstract_unit'

class IPv6IntegrationTest < ActionDispatch::IntegrationTest
Copy link
Member

Choose a reason for hiding this comment

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

Can't we just include this test in n existent test file?

@rafaelfranca
Copy link
Member

@al2o3cr good question. I can't think in any special reason.

@joallard
Copy link

joallard commented Jun 9, 2015

TestRoutingMapper is ~4500 lines long, maybe this would help split it up a little? Otherwise, is there some other work needed? Seems to me that Rails' IPv6 support is contingent on this patch.

@JasonHerr JasonHerr mentioned this pull request Aug 25, 2015
Closed
@robin850
Copy link
Member

@cmdrclueless : Any chance to address Rafael's feedback please ? Thanks for the patch so far!

@cmdrclueless
Copy link
Contributor Author

@robin850 to which comment are you referring? Assuming it's about the rational of putting IPv6 testing in a separate file... well @joallard more or less hit the nail on the head. It seemed reasonable to put the actual test in a separate file to limit the scope to the actual problem I was addressing.

rafaelfranca added a commit that referenced this pull request Sep 1, 2015
@rafaelfranca
Merge pull request #19788 from cmdrclueless/actionpack_http_url_ipv6
c47c1d2 
Fix broken IPv6 addresses handling
@rafaelfranca rafaelfranca merged commit c47c1d2 into rails:master Sep 1, 2015
rafaelfranca added a commit that referenced this pull request Sep 1, 2015
@rafaelfranca
Merge pull request #19788 from cmdrclueless/actionpack_http_url_ipv6
e7c68d0 
Fix broken IPv6 addresses handling
@rafaelfranca
Copy link
Member

Backported in e7c68d0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
actionpack needs feedback
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@cmdrclueless @al2o3cr @rafaelfranca @joallard @robin850