params = ActionController::Parameters.new(comment: 'some text')

params.require(:comment).permit(:text)

Is this a typo and did you mean permit(:comment)? It's inconsistent with your other comment.

I know that such code will return String value 'some text' at require(:comment) step, but after this we got 'some text'.permit(:comment) and error like:

params = ActionController::Parameters.new(comment: 'some text')

params.require(:comment).permit(:text)

Is this a typo and did you mean permit(:comment)? It's inconsistent with your other comment.

I know that such code will return String value 'some text' at require(:comment) step, but after this we got 'some text'.permit(:comment) and error like:

No, code is correct. I simulated situation when form was manually changed and request with params was fabricated. During such actions we guaranteed to get an error.
Cause in this line we didn't do any checks of value type (or something like) and pass as is:

Ps. of course i need :text value

I see possible fix as:

  if value.is_a?(ActionController::Parameters)

Looking at the require code:

def require(key)
  return key.map { |k| require(k) } if key.is_a?(Array)
  value = self[key]
  if value.present? || value == false
    value
  else
    raise ParameterMissing.new(key, @parameters.keys)
  end
end

Basically, in your example value ends up being a String. In a correct usage, it's an instance of ActionController::Parameters. Why not open a PR for this, implementing the behavior you want and see what others think?

Just be aware that the expected behavior according to the API is "If it's present, returns the parameter at the given key, otherwise raises an ActionController::ParameterMissing error". So the API is not making promises on the type of the returned parameter, only that it will be returned. There also a quite old PR where the false condition was added to ensure that this behavior was correct, so your change would be basically undoing that PR, since false would no longer be a valid return.

@brenogazzola I understand what you mean.
But changing of default behaviour by rails convention should be approved by rails core DEV's firstly.
I think rails community need more attention to this trouble, because this issue covers thousands rails app's and confuse dev's who are sure (according docs https://api.rubyonrails.org/classes/ActionController/Parameters.html) that permit method can be safely chained to require method without any additional checks.

I had assumed from your issue that your intent was to produce a better error message when the format of params did not match what the controller expected.

IMO, if the controller expects comment to contains a text, and the form only sends the comment, raising an error is what I’d like Rails to do, so I know the exact place where the problem is.

How would safe chaining be used? I’m not asking as a criticism, but as legitimate curiosity since I’ve never thought about using strong_params as anything other than a formal contract between controller and view on how to exchange data.

@brenogazzola I just trying to say that construction below (params.require().permit()) (which rails doc recommended https://api.rubyonrails.org/classes/ActionController/StrongParameters.html) is unsafe and produce errors in 100% cases with fake data and didn't gave ability to directly use in code (as I want and as expected), without additional checks for preventing this attack, that's it. VERY simple construction.

# params = ActionController::Parameters.new(person: { name: 'Name'}) # OK
# params = ActionController::Parameters.new(person: 'fake data') # fake data, that give exception in 100% cases

person_params = params.require(:person).permit(:name)
Person.create(person_params)

Alright, in this case I'll recommend you cross post this on the rubyonrails-core channel of the official forums. It will increase exposure and allow other developers who might be interested in this change to give input and provide arguments in favor of it. This was recommended to me in the past by one the Rails members, and helped me successfully get a breaking change merged.

You might also give another thought to the PR suggestion. It’s what Rails core members themselves recommend.

This is also related to another recent issue: #42942

This issue has been automatically marked as stale because it has not been commented on for at least three months.
The resources of the Rails team are limited, and so we are asking for your help.
If you can still reproduce this error on the 6-1-stable branch or on main, please reply with all of the information you have about it in order to keep the issue open.
Thank you for all your contributions.