ActiveRecord::Base#toggle! - bad implementation or documentation

ActiveRecord::Base#toggle! is used to toggle an attribute true or false, and immediatly save it. It is implemented to call the toggle method, and then call update_attribute to save to the database.

The problem I see with this is that there is no validation, when toggling a boolean. In my opinion this is wrong, and there should be validation. I've heard people say this behaviour was correct, and that's how they wanted it, in which case the documentation is incomplete and can lead to unexpected results.

def toggle!(attribute) toggle(attribute).update_attribute(attribute, self[attribute]) end You tell me, a patch to make toggle! validate or a documentation patch to say toggle! doesn't validate when saving.

I have a ticket open at http://dev.rubyonrails.org/ticket/11098 about this.

Regards,

Jan

I'd personally love to see #toggle! removed from ActiveRecord entirely. I can't think of a single use case for #toggle! that isn't bug-prone, or that wouldn't be better implemented by setting flags to explicit values.

Tammer

I’d personally love to see #toggle! removed from ActiveRecord

entirely. I can’t think of a single use case for #toggle! that isn’t

bug-prone, or that wouldn’t be better implemented by setting flags to

explicit values.

I heartily second this. I haven’t used it since I ran into a situation where I was mis-guessing the initial state of a boolean.

::Jack Danger

Sure, I can: boolean fields that can be toggled by administrators. I use a fair number of these in a project I'm doing. #toggle! is exactly what I would have implemented had Rails not had the feature to begin with.

James

I'd call that a bad practice, and I think it's commonly used only because of the existence of AR's #toggle! method.

The "toggling" an admin flag via a web form is really "setting the flag to true or false, depending on what the form thinks it currently is". The problem with using #toggle! here is that when two administrators both load the form and then both click the "publish" button (or whatnot), the record will end up being unpublished.

The correct solution is very easy in a restful application, where the "publish" link should put params[:record][:published] = true to the update action. But even in non-restful applications, having the link post to #publish and #unpublish, depending on the state of the record when the page is loaded is much more explicit, and less error prone.

This, and the fact that #toggle! ignores validates (the original point of the thread), makes #toggle! stick out as a very non-rails way of doing things.

The existence of #toggle! encourages its use. It should be removed, if only to guide new rails programmers toward best practices.

Tammer

Of course, the correct solution would be not to use a link (or GET) to do such an action in the first place. Other than, agreed.

//jarkko

Also, if for whatever reason the request was retryed (by a proxy perhaps) even a single user's actions could result in an unknown final toggle state.

+1 for removal

Also, if for whatever reason the request was retryed (by a proxy perhaps) even a single user's actions could result in an unknown final toggle state.

+1 for removal

Proxies don't retry non-idempotent requests. If they did we'd have a whole pile of other issues.

Base#toggle works just fine for its intended use case, toggling a boolean value. There are dozens of cases where this is perfectly valid and the concurrency issues raised aren't the end of the world. tadalist's todo lists for example.

Some patches to improve the documentation would be greatly appreciated, but the feature's definitely not 'useless' :slight_smile: