GetIp#to_s should never return nil. That's icky. #3640

Merged
merged 1 commit into from Nov 15, 2011

Projects

None yet

4 participants

@indirect
Ruby on Rails member

Okay, so my previous fix for this was to acknowledge that GetIp#to_s could return nil. And that's really, really awful. So this fix returns the first IP value in REMOTE_ADDR instead of nil, and stops assuming that to_s could return nil. All the tests pass, and I'd welcome any feedback on how to make this better/cleaner. Thanks!

@indirect
Ruby on Rails member

@spastorino code review call <3 <3 <3

Ruby on Rails member

@indirect sorry :( but I'm busy as hell, can you provide a PR and ping someone else

@wycats wycats merged commit 6491aad into rails:master Nov 15, 2011
@jonleighton
Ruby on Rails member

Hi,

There are still failures due to these changes. The fix from the other day fixed one thing but introduced new failures.

Before: http://travis-ci.org/#!/rails/rails/builds/306399
After: http://travis-ci.org/#!/rails/rails/builds/306654

(Ignore the sqlite3 failures - looks like the kernel was killing the process in those)

After this merge there are even more failures:

http://travis-ci.org/#!/rails/rails/builds/310448

I am going to revert this merge for now, please look into the existing failure when you have a chance and then after that we can look at getting this in. Thanks :)

Jon

@jonleighton jonleighton added a commit that referenced this pull request Nov 15, 2011
@jonleighton jonleighton Revert "Merge pull request #3640 from indirect/remote_ip"
This reverts commit 6491aad, reversing
changes made to 83bf0b6.

See #3640 (comment) for
explanation.
8d1a2b3
@indirect
Ruby on Rails member

Boo. Thanks for following up. FWIW, I did run the entire test suite and have it pass (on my machine) with the fix from the other day. I'll look into this when I get the chance and let you know.

@indirect indirect added a commit to indirect/rails that referenced this pull request Nov 16, 2011
@indirect indirect Revert "Revert "Merge pull request #3640 from indirect/remote_ip""
This reverts commit 8d1a2b3, because I have fixed the issues this commit caused in the next commit.
f05ccf8
@georgeG georgeG added a commit to georgeG/rails that referenced this pull request Nov 27, 2011
@georgeG georgeG Merge remote-tracking branch 'upstream/master'
* upstream/master: (102 commits)
  Revert "Merge pull request #3640 from indirect/remote_ip"
  refactor test_multiple_of
  GetIp#to_s should never return nil. That's icky.
  add test for bug fixed in 4f2bf64
  Return the calculated remote_ip or ip
  memoize the relatively expensive remote IP code
  cleaner names
  Speed up attribute invocation by checking if both name and calls are compilable.
  Add a note to REALEASING_RAILS about testing the gem locally before releasing
  rake release should push the tag
  use any? instead of !empty?
  Sync changelog entry
  Add note about checking postgres tests before release
  Added therubyrhino to default Gemfile under JRuby
  Add note about syncing CHANGELOGs
  Sync CHANGELOGs from 3-1-stable
  Upgrade Sprockets to 2.1.0
  remove ignored flag, fixes warnings
  turns out the tests expect remote_addrs.first
  correctly raise IpSpoofAttackError message
  ...
93277bb
@georgeG georgeG added a commit to georgeG/rails that referenced this pull request Nov 27, 2011
@georgeG georgeG Merge remote-tracking branch 'upstream/master'
* upstream/master: (25 commits)
  It should be @calculated_ip not @calculate_ip
  No need to `readlines` then `join`, just use `read` ❤️
  Fix impractical I18n lookup in nested fields_for
  Initialize our instance variables.
  No need to `readlines` then `join`, just use `read` ❤️
  Adding a deprecation warning for use of the schema_info table.
  Join method uses empty string by default, so remove it
  dbfile isn't supported anymore, so remove
  Reduce schema format tests
  Move conditionals to separate tasks so they can be reused.
  `ActiveRecord::Base#becomes` should retain the errors of the original object.
  the object itself isn't the IP, #to_s is the IP
  :facepalm: Request#remote_ip has to work without the middleware
  Revert "Revert "Merge pull request #3640 from indirect/remote_ip""
  removing some useless conditionals
  Switch from marshal format to plain text for the encoding conversions dump. This is for windows compatibility. Fixes #3644.
  fixing tests on PG
  bundler treats trunk ruby as ruby 1.9, hack around that for now
  Substituted RailsCommands for Rails::Commands
  Changed Commands module to RailsCommands.
  ...
d46826d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment