update_attribute, update_column and update_columns should update readonly attributes #7688

Merged
merged 0 commits into from Dec 8, 2012

Conversation

Projects
None yet
3 participants
@VadimPushtaev

It was decided two year ago in c96a505 that "update_attribute should not update readonly attributes". I believe it's missleading and should be changed.

I wrote an issue asking whether it's normal or not: rails/rails#7679. As you can see, it was recommended to me to try sending pull request.

Reasons I believe update_attribute, update_column and update_columns should update readonly attributes:

  1. All validations are skipped in this methods. Readonly check really looks like validation.

  2. update_attribute is kind of more 'raw' version of = operator, but in this case I can do

    minivan.color = 'new_color'
    minivan.save

    without getting exceptions, but can't do

    minivan.update_attribute :color, 'new_color'

    It's pretty weird, in this case = behaves more raw than update_attribute.

  3. Readonly control is relatively low-level:

    # Filters the primary keys and readonly attributes from the attribute names.
    def attributes_for_update(attribute_names)
     attribute_names.select do |name|
       column_for_attribute(name) && !pk_attribute?(name) && !readonly_attribute?(name)
    end
    end

    And here is one more oddity: we don't raise exception for pk_attribute, but do for readonly_attribute in update_attribute, update_column and update_columns.

I'm open for discussion. Please, let me know where I'm wrong :)

@lunks

This comment has been minimized.

Show comment Hide comment
@lunks

lunks Sep 25, 2012

Contributor

For me, it's reasonable for update_column to update read only attributes, not the others.

Contributor

lunks commented Sep 25, 2012

For me, it's reasonable for update_column to update read only attributes, not the others.

@VadimPushtaev

This comment has been minimized.

Show comment Hide comment
@VadimPushtaev

VadimPushtaev Sep 26, 2012

They do not really update anything, because of attributes_for_update. Now they just don't raise strange exception they don't need to.

They do not really update anything, because of attributes_for_update. Now they just don't raise strange exception they don't need to.

@frodsan

This comment has been minimized.

Show comment Hide comment
@frodsan

frodsan Oct 26, 2012

Contributor

If this gets merged, you should update docs too.

Contributor

frodsan commented Oct 26, 2012

If this gets merged, you should update docs too.

@VadimPushtaev

This comment has been minimized.

Show comment Hide comment
@VadimPushtaev

VadimPushtaev Dec 8, 2012

Sorry, I've broken something trying to change branch. Just use #8464 now.

Sorry, I've broken something trying to change branch. Just use #8464 now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment