Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Compact array of values added to PermissionSet instance #6668

Merged
merged 1 commit into from Jun 8, 2012

Conversation

Projects
None yet
3 participants
Contributor

pomnikita commented Jun 7, 2012

It is necessary to compact arrays with nil values.

For example, if we enforce

  config.active_record.whitelist_attributes = true

on_load hook will call attr_accessible(nil) on ActiveRecord
and it appears that ActiveRecord class attribute _accessible_attributes now is set to

{ :default => #<ActiveModel::MassAssignmentSecurity::WhiteList: {""}> }

(https://github.com/rails/rails/blob/master/activemodel/lib/active_model/mass_assignment_security.rb#L183)

And when we try to set attr_accessible on any other model, for example

  class User < ActiveRecord
    attr_accessible :admin
  end

and call accessible_attributes, we can get a blank value together with 'admin'

  User.accessible_attributes = ["", "admin"]

drogus added a commit that referenced this pull request Jun 8, 2012

Merge pull request #6668 from pomnikita/master
Compact array of values added to PermissionSet instance

@drogus drogus merged commit 41d6371 into rails:master Jun 8, 2012

Contributor

pomnikita commented Jun 8, 2012

Hi drogus, thanx for merging. Is it possible to merge it into current stable 3.2 ?

Member

drogus commented Jun 8, 2012

Frankly I'm not sure. It's fine for me, but technically speaking it changes a behavior of accessible_attributes. Probably no one relies on the fact that it returns blank string as one of the results, so it should be safe, but I would like to get feedback.

/cc @tenderlove @josevalim

Owner

rafaelfranca commented Jun 9, 2012

I rely in this behavior, but only for tests propose. It is fine to me, but I'm afraid to add another regression to the 3-2-stable branch.

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