Use `Array#wrap` instead `Array()` #13557

Merged
merged 2 commits into from Jan 1, 2014

Projects

None yet

3 participants

@gmarik
Contributor
gmarik commented Dec 31, 2013
  • since Array() calls to_a on a subject
  • the intent is to 'wrap' subject into an array
@gmarik gmarik Use `Array#wrap` instead `Array()`
- since `Array()` calls `to_ary` or `to_a` on a subject
- the intent is to 'wrap' subject into an array
8ea00ed
@josevalim
Member

Thanks but I don't see what we are gaining with this change. Array#wrap exists mostly to handle the string with new line cases, which doesn't seem to matter here. If this change is important, shouldn't we have a test case?

@gmarik
Contributor
gmarik commented Dec 31, 2013

@josevalim thanks for the quick response!

I think it worth changing as current code assumes that result of record.send(attribute):

  1. doesn't respond to_a or
  2. if responds to_a then also responds to marked_for_destruction?

The case 2) is flawed obviously.

IMO the intent here is to have associated_records in an array and not to convert association into an array as it may result in losing actual association object and getting an array representation whatever may be.

Let me know!

@gmarik
Contributor
gmarik commented Dec 31, 2013

So without Array.wrap if an associated object defines to_a method it gets called and error is raised:
See example test

PresenceValidationTest#test_validates_presence_doesnt_convert_to_array:
NoMethodError: undefined method `marked_for_destruction?' for "(/)":String
    rails/activerecord/lib/active_record/validations/presence.rb:11:in `block (2 levels) in validate'
    rails/activerecord/lib/active_record/validations/presence.rb:11:in `each'
    rails/activerecord/lib/active_record/validations/presence.rb:11:in `all?'
    rails/activerecord/lib/active_record/validations/presence.rb:11:in `block in validate'
    rails/activerecord/lib/active_record/validations/presence.rb:6:in `each'
    rails/activerecord/lib/active_record/validations/presence.rb:6:in `validate'
    rails/activesupport/lib/active_support/callbacks.rb:283:in `_callback_before_2'
    rails/activesupport/lib/active_support/callbacks.rb:387:in `_run__2408498541307108321__validate__callbacks'
    rails/activesupport/lib/active_support/callbacks.rb:80:in `run_callbacks'
    rails/activemodel/lib/active_model/validations.rb:373:in `run_validations!'
    rails/activemodel/lib/active_model/validations/callbacks.rb:106:in `block in run_validations!'
    rails/activesupport/lib/active_support/callbacks.rb:373:in `_run__2408498541307108321__validation__callbacks'
    rails/activesupport/lib/active_support/callbacks.rb:80:in `run_callbacks'
    rails/activemodel/lib/active_model/validations/callbacks.rb:106:in `run_validations!'
    rails/activemodel/lib/active_model/validations.rb:314:in `valid?'
    rails/activerecord/lib/active_record/validations.rb:70:in `valid?'
    test/cases/validations/presence_validation_test.rb:66:in `block in test_validates_presence_doesnt_convert_to_array'
    rails/activesupport/lib/active_support/test_case.rb:64:in `assert_nothing_raised'
    test/cases/validations/presence_validation_test.rb:66:in `test_validates_presence_doesnt_convert_to_array'
@rafaelfranca rafaelfranca merged commit aaf1af3 into rails:master Jan 1, 2014

1 check failed

default The Travis CI build failed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment