Skip to content
This repository was archived by the owner on Aug 17, 2017. It is now read-only.

Allow parameter filters to match multi-parameter attributes #17

Closed
wants to merge 1 commit into from

Conversation

jpignata
Copy link

We're using the DateHelper in our application so in some places our permit list includes things like:

def book_params
  params[:book].permit( 
    :"published_at(1i)",                                                      
    :"published_at(2i)",                                                      
    :"published_at(3i)",                                                      
    :"published_at(4i)",                                                      
    :"published_at(5i)",
   ...
  )
end

This change matches multi-parameter attributes based upon the head of the key so you can just include :published_at in the above example.


def multi_param_keys_for(filter)
keys.select { |key|
key.index("#{filter}(") == 0 && key.index(")") == key.length - 1
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just use regexp for this?

keys.select do |key|
  key.match /#{filter}\(\d+i\)/
end

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#index was the fastest way to figure it out when I measured different approaches. I agree it's cleaner with a Regexp though.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i found regexp are almost always faster than anything when it comes to strings. maybe with other regexp it will be faster:

key.match /\A#{filter}\(\d{1}i\)\z/i

@dhh
Copy link
Member

dhh commented Jun 29, 2012

I'd rather we just default to permit :published_at will also just automatically allow :published_at(x) -- instead of all this extra configuration. If you want to take a stab at a pull request that does that, please open a new ticket. I posted the same message to pull request #21.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants