why revert "adding accessible option to allow for allowing mass assignments"?

I noticed that Pratik just reverted the ":accessible option to allow
for allowing mass assignments" with the following commit:
http://github.com/rails/rails/commit/9994f0d90248db7d7eae36f0b597a15e8a427612

What was the reason behind this reversion?

I've been using the feature and it works great for my needs.

Best,

Zack Chandler
http://depixelate.com

Hey Zach,

The :accessible change was not complete and still needs some more work to be usable. There was no conclusion about how we want updates to work - http://groups.google.com/group/rubyonrails-core/browse_thread/thread/4049b4b313fa8be2/730dbd7ad5e7ccc0

As 2.2 is very close, I didn’t want any half baked changes to be in stable release. But I’d be happy to apply the patch again, and work incremently after we have a 2.2 branch.

Pratik,

Got it - I figured it was prep time for 2.2... I for one would like
to see this reapplied and iterated on after 2.2 is tagged as it really
helps out with multi-model forms.

Best,
Zack

I like this feature too, but agree that it wasn't ready for prime time
yet. I just put up a patch for my own take on how to do this. It's
not the whole story, but I wanted to get it up there so we can talk
about it:

http://rails.lighthouseapp.com/projects/8994/tickets/1031

Here's a plugin extracted from one of our apps and then extended for
Ryan Bates's complex-form-examples.
We only needed it for has_many and has_one associations atm, so I
haven't worked/tried it on others yet.

The plugin exists of 3 parts:
- AutosaveAssociation, which as the name implies automatically saves
associations, not only on create as is the default.
- NestedParams, which is my take on this problem. It creates/updates/
destroys associations. This uses AutosaveAssociation.
- NestedParamsFormBuilder, which is a subclass of FormBuilder that
adds code to #fields_for for handling NestedParams enabled models.

Obviously this is a wip, but it might be interesting for the
discussion as well.

http://github.com/alloy/complex-form-examples/tree/alloy-nested_params/vendor/plugins/has_autosave_and_nested_params

Cheers,
Eloy

I think one of the problem's is that there are so many ways the
interface can be implemented. That is, the interface for setting the
associated attributes. I'm starting to document them here.

http://gist.github.com/10793

Josh, I haven't added yours yet as I don't fully understand it. Is it
close to Approach 2? Let me know and I'll add it.

We should weigh the pros and cons of each approach.

Regards,

Ryan

Are there any plans to allow this type of attribute nesting to go
beyond just 2 levels deep? For example, ":blog => {:posts =>[1 =>
{:comments => [1,2,3]}, 2 => {:comments => [4,5,6]}, ... ]}" instead
of just ":blog => {:posts => [1,2,3]}"

Brad

Yes, in a sense that comes "for free" because we're just doing mass
assignment in the nesting. You can go as deep as you want as long as
it's supported through mass assignment. This is one reason why this
feature should need to be enabled manually instead of always on by
default (for security).

I've been giving some thought to the interface. One question I keep
coming back to is: how much do we want to support multi-model forms
through this? I think that is the primary use case, but this has other
uses as well. Active Record is not specific to web interfaces and
therefore shouldn't be tied too heavily to them.

IMO Approach 1 (at http://gist.github.com/10793) is the cleanest
approach if we're just doing this all in Ruby. This is also fairly
easy to use in a web form. However, there are a few major drawbacks
when doing so:

1. The resulting HTML is not validatable due to the duplicate form
field names.
2. It's more difficult to work with the fields in Javascript because
of the duplicate form field names.
3. It's impossible to nest associations multiple layers deep because
it gets confused when there are multiple "[]" in the field name.
4. Checkboxes (and I think radio buttons?) are nearly impossible to
get working.

Each of these problems stem from the fact that not each field has a
unique name/identifier. Therefore when it comes to multi-model form
fields, I'm more inclined to go with Approach 3 because each record
has its own unique key/identifier. Theoretically that will solve all
of the problems above.

That said, if you ever need to set associated attributes in other
scenarios (maybe preparing data in test cases), then Approach 3 is not
clean or optimal.

For what it's worth, in my project I've been using something like a
combination of Approaches 1 & 3 (from http://gist.github.com/10793):

{
  tasks =>
    {
      '1' => { :id => '3', :name => 'Foo' },
      '2' => { :name => 'Bar' },
      '3' => { :name => 'Baz' },
    }
}

As with Approach 1, you can tell new records from existing records by
the lack of an :id attribute.

As with Approach 3, this supports nesting associations multiple levels
deep because it uses a hash rather than arrays.

The keys in the tasks array (1, 2 & 3) come from the counter variable
created by render :partial, :collection (or from an each_with_index
loop).

Regards
Dave.

Cool, Dave, nice approach. I added it to the gist.
http://gist.github.com/10793

Out of curiosity, do you have a "new task" link on the form which
dynamically adds new fields using Javascript? Is it difficult to
generate the auto-incrementing number with this?

Regards,

Ryan

Using the presence of the :id attribute to distinguish old vs new
records seems ok, but it might get wonky for edge cases that use a
different primary key. I think I do like that approach. The thing
that is important that some of the gisted approaches miss is being
able to order new records in the form. It can get seriously confusing
for users if data shifts around form one place to another if they
failed a validation and need to correct something.

I'm not particularly attached to using two different faux accessors
for create vs update. It's nice and clean, but really any way that
lets you tell create vs update is fine by me.

--josh

Oh yeah, I should add a refinement of "Approach 4":

{
   :tasks_params =>
     {
       '1' => { :id => '3', :name => 'Foo' },
       '2' => { :id => '17', :_delete => '1', :name => 'Bob' },
       '3' => { :name => 'Bar' },
       '4' => { :name => 'Baz' }
     }
}

The presence of a :_delete param is used to indicate that an old
record should be deleted. This can be done simply with a checkbox or
a JavaScript control. I think this works much better than trying to
delete records that just happen to be missing from the hash, since
that can get ugly if you are paginating a bunch of records or
otherwise subsetting e the list.
And I do think the _params (or _attributes) suffix on the name is
important.

Anyway, I'll try and get my proposed patch mashed around for this new
arrangement, and see if we can get a good form_for mod to support this
as well.

--josh

Oh yeah, I should add a refinement of "Approach 4":

{
  :tasks_params =>
    {
      '1' => { :id => '3', :name => 'Foo' },
      '2' => { :id => '17', :_delete => '1', :name => 'Bob' },
      '3' => { :name => 'Bar' },
      '4' => { :name => 'Baz' }
    }
}

The presence of a :_delete param is used to indicate that an old
record should be deleted. This can be done simply with a checkbox or
a JavaScript control. I think this works much better than trying to
delete records that just happen to be missing from the hash, since
that can get ugly if you are paginating a bunch of records or
otherwise subsetting e the list.

I really can't think of a common use case for this scenario.
It seems to me that when you're paginating a form, or any other edge case,
you shouldn't rely on any naive solution, but rather solve it yourself in your controller/model.

This is why I personally feel that destroying a record should be off by default.

But for simple common use cases, where you might have removed
a child from the DOM, the naive/simple approach I took in my plugin is good enough IMO.

Using the presence of the :id attribute to distinguish old vs new
records seems ok, but it might get wonky for edge cases that use a
different primary key. I think I do like that approach. The thing
that is important that some of the gisted approaches miss is being
able to order new records in the form. It can get seriously confusing
for users if data shifts around form one place to another if they
failed a validation and need to correct something.

This is a very valid point which I have fixed in:
http://github.com/alloy/complex-form-examples/commit/4c740d5d8f5f99c0bfa5c3f38162ef26329c7cf0
Thanks for bringing that up.

I'm not particularly attached to using two different faux accessors
for create vs update. It's nice and clean, but really any way that
lets you tell create vs update is fine by me.

I'm sorry, but my English is not good enough to follow this bit.
Could you maybe explain it some more or in other words?

Eloy

Oh yeah, I should add a refinement of "Approach 4":

{
:tasks_params =>
   {
     '1' => { :id => '3', :name => 'Foo' },
     '2' => { :id => '17', :_delete => '1', :name => 'Bob' },
     '3' => { :name => 'Bar' },
     '4' => { :name => 'Baz' }
   }
}

The presence of a :_delete param is used to indicate that an old
record should be deleted. This can be done simply with a checkbox or
a JavaScript control. I think this works much better than trying to
delete records that just happen to be missing from the hash, since
that can get ugly if you are paginating a bunch of records or
otherwise subsetting e the list.

I really can't think of a common use case for this scenario.
It seems to me that when you're paginating a form, or any other edge
case,
you shouldn't rely on any naive solution, but rather solve it yourself
in your controller/model.

This is why I personally feel that destroying a record should be off
by default.

But for simple common use cases, where you might have removed
a child from the DOM, the naive/simple approach I took in my plugin is
good enough IMO.

This isn't a naive solution, but one that worked well for us in a rather complicated workflow. Making the controller do extra stuff to handle deletes outside of the save transaction is annoying and problematic. Letting the model handle deletes as part of the same operation means the entire change can be handled in one transaction, which simplifies things a lot. We found that using a _delete field worked best for us. Noticing that all fields were blank was a lot of work for both the user and our code, and didn't work at all if the model included radio buttons or select boxes without the blank option.

Using the presence of the :id attribute to distinguish old vs new
records seems ok, but it might get wonky for edge cases that use a
different primary key. I think I do like that approach. The thing
that is important that some of the gisted approaches miss is being
able to order new records in the form. It can get seriously
confusing
for users if data shifts around form one place to another if they
failed a validation and need to correct something.

This is a very valid point which I have fixed in:
http://github.com/alloy/complex-form-examples/commit/4c740d5d8f5f99c0bfa5c3f38162ef26329c7cf0
Thanks for bringing that up.

I'm not particularly attached to using two different faux accessors
for create vs update. It's nice and clean, but really any way that
lets you tell create vs update is fine by me.

I'm sorry, but my English is not good enough to follow this bit.
Could you maybe explain it some more or in other words?

Sorry about that, I was using non-standard jargon. I use "faux accessor" to describe a model method that pretends to be an attribute accessor for the purpose of using a form to edit something that isn't a primitive attribute. And in my proposed patch for parameterized associations I use two different faux accessors for the API, e.g. create_tasks_params and update_tasks_params.

I think he's referring to Approach 2 (of http://gist.github.com/gists/10793
) which has both a new_task_attributes and existing_task_attributes
accessor. I'm not very fond of this either, especially when you're
setting it directly through Ruby and not using form params.

BTW Eloy, I saw your idea of using the timestamp when adding new task
records (through javascript). Genius!

What if we support both Approach 1 and Approach 4? Here's the logic in
pseudo code.

I really can't think of a common use case for this scenario.
It seems to me that when you're paginating a form, or any other edge
case,
you shouldn't rely on any naive solution, but rather solve it yourself
in your controller/model.

This is why I personally feel that destroying a record should be off
by default.

But for simple common use cases, where you might have removed
a child from the DOM, the naive/simple approach I took in my plugin is
good enough IMO.

This isn't a naive solution, but one that worked well for us in a
rather complicated workflow.

I was referring to my implementation as being naive, not yours :slight_smile:

Making the controller do extra stuff to
handle deletes outside of the save transaction is annoying and
problematic. Letting the model handle deletes as part of the same
operation means the entire change can be handled in one transaction,
which simplifies things a lot. We found that using a _delete field

worked best for us.

Indeed I would solve this in the model as well.

I still don't see the pagination etc as a common use case.
However, adding the option to check for the presence of "_delete" => "1",
seems an easy/clear enough option as well.

Noticing that all fields were blank was a lot of
work for both the user and our code, and didn't work at all if the
model included radio buttons or select boxes without the blank option.

Just to be sure that we are talking about the same, and not different approaches from the gist :slight_smile:
My implementation has 2 distinct options;
- :reject_empty : Which doesn't create new records for empty hashes.
- :destroy_missing : Which destroys records if they're missing in the attributes hash.
Both of these are off by default.

Sorry about that, I was using non-standard jargon. I use "faux
accessor" to describe a model method that pretends to be an attribute
accessor for the purpose of using a form to edit something that isn't
a primitive attribute. And in my proposed patch for parameterized
associations I use two different faux accessors for the API, e.g.
create_tasks_params and update_tasks_params.

Thanks for the clarification. I agree.

As I always prefer comparing implementation over discussion,
I will take some time one of the next few days to turn my plugin into a patch.
I will also include the option to mark a record to be destroyed with "_delete".

Eloy

I think he's referring to Approach 2 (of http://gist.github.com/gists/10793
) which has both a new_task_attributes and existing_task_attributes
accessor. I'm not very fond of this either, especially when you're
setting it directly through Ruby and not using form params.

Thanks for clarifying.

BTW Eloy, I saw your idea of using the timestamp when adding new task
records (through javascript). Genius!

Thanks :slight_smile:

What if we support both Approach 1 and Approach 4? Here's the logic in
pseudo code.

--
def tasks=(new_tasks)
if new_tasks is a hash
   grab values (as array) and sort by keys
   call self.tasks= with new values array
else
   for each task in new_tasks
     if task is hash
       if task has id key
         update existing task matching id
       else
         add new task with hash attributes
       end
     else
       add task
     end
   end
end
end
--

Supporting both hashes and arrays opens up a lot of flexibility.
Generally you'd use an array when dealing directly in Ruby, but a hash
when going through a form.

I agree. I think I already support what you mean.
But maybe it would be an idea if you could send me a test which tests the difference
in behaviour between a Hash and a Array so I can verify?

Cheers,
Eloy

Sure thing, I'll work on getting some real code together. I think the
primary difference between this approach and yours is that it uses
the :id key to distinguish existing records. I like this because it
removes the need for the magic "new_" hash key and is almost identical
to passing an array. The only thing the keys are used for is sorting
the values.

Regarding deleting. It depends on whether or not we use the "tasks="
accessor method. Rails already has this implemented, along with logic
on how deleting happens. AFAIK, it currently "resets" the tasks
association. That is, tasks which aren't mentioned there are removed
from the association automatically. Whether they are deleted entirely
or not depends on the :dependent option in has_many (I'm assuming). I
think we should follow that same logic.

Look at it this way, when you're setting "tasks=", the values could be
represented as either hashes or actual Task instances (which are
currently supported). It makes sense that you can mix and match them
and not get strange behavior in how deleting happens.

I was leaning toward Approach 1 & 4 because of the much cleaner
interface. However, it just hit me that this feature will not be
enabled by default (for security), so that kind of puts a damper on
using it directly in Ruby code (such as in test cases to create
records). I'm convinced that 99% of the time this will only be used in
multi-model forms so we should cater to that.

On that note, I've added Josh's approach (from his patch) to the gist.
See Approach 5.
http://gist.github.com/10793

I like this because the implementation is very simple. It does not
override the "tasks=" method or add any extra magic there. New records
are created with a hash so they can be sorted and have a unique
identifier when passing through the form.

Regards,

Ryan