Handle nil form_with model argument

[cjilbert504]

Motivation / Background

First, thank you to everyone here for taking the time to read this description.

I would like to propose a possible change to the form_with method which would help provide, in my opinion, better debugging around an issue that can be encountered when passing a nil object to the model: argument in the form_with method.

I have come across this issue a few times myself and I know others that have as well. The most recent encounter was from a newer Rails developer that myself and others have been helping along in the process of learning Rails.

The issue actually shows up in two ways, the first is if the form is on an index page, having nil passed to the :model argument this will raise an ActionController::ParameterMissing error. The other is in the default scenario of being on the new page and having nil passed to the :model argument. In this scenario, an ActionController::RoutingError (No route matches [POST] "/posts/new") (using Post model as an example). This happens because of Rails falling back to building the URL from the current page in this scenario.

Going back to the first form of this causing an error, I would like to outline the flow of the issue so that everyone can (hopefully) have a better understanding of this issue.

---

Junior Dev: I'm having an issue using strong parameters. Has there been a change in rails 7 when using strong parameters?

def url_link_params
  params.require(:url_link).permit(:url)
end

where the model name is UrlLink. When I try to submit my form, I get the following error:

ActionController::ParameterMissing (param is missing or the value is empty: url_link)

When I remove require and just use params.permit(:url), I can submit the form.
I'm confused, I setup my form with form_with(model: url_link) and have a url input field form.url_field :url.

Me: I see that you have a mismatch in instance variable naming between the instance variable declared in the controller action versus what is being passed in as a local to the form partial:

def index
  @link = UrlLink.new
end

<%= render partial: "form", locals: { url_link: @url_link } %>

Junior Dev: Thanks, I fixed the mismatch and my form and controller are working now.

With this context, I think it would have been a better debugging experience for the junior if instead of the ActionController::ParameterMissing error being raised thus causing the junior to think it was a strong params issue (which it sort of is I admit) I think it would be more helpful to raise an ArgumentError when passing in a nil object to the model: argument in form_with.

---

Detail

This PR changes the default value of the model: argument in form_with to false instead of nil and raises an ArgumentError in the event that the argument is nil.

def form_with(model: false, other_args)
  raise ArgumentError, "Form model object is nil" if model.nil?
end

I tried to think reasons/scenarios of why/where false wouldn't be a suitable replacement for nil as the default value for the model: argument but I couldn't think of any (I'm not saying there isn't a case though and would love to hear so if there are any).

It also changes a local variable that is set in a case statement in the form_for method from nil to false which is needed to allow form_for to continue to call form_with successfully.

Additional information

Thank you for taking the time to read this and for any thoughts or feedback about it.

Checklist

Before submitting the PR make sure the following are checked:

• This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
• Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
• Tests are added or updated if you fix a bug or add a feature.
• CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

[excid3]

@rafaelfranca when we were talking about issues that beginners face at Rails World, this is one of the most common we've seen. Instance variables returning nil instead of an undefined variable error makes it confusing to beginners who aren't the most careful with variable naming yet.

[mattpolito]

I was under the impression that when form_with was released, it was to eventually be a single interface replacement for form_for and form_tag. This would mean that it's supposed to work without a model. You could just give it a url and get a form helper object to work with.

I think the PR is great but the functionality would only apply to form_for.

[mattpolito]

@excid3 I completely agree with the instance variable headaches for beginners. It's one of the reasons we developed decent_exposure. It encapsulates what they're trying to do with an instance variable into a view helper. Since it gets turned into an actual method, Ruby gives way more help with things like a typo.

[cjilbert504]

@mattpolito - Hey there!

This would mean that it's supposed to work without a model. You could just give it a url and get a form helper object to work with.

This change still allows that functionality. This is handling when you make a call something like form_with(model: @post) but @post is nil. I don't know when you would ever intentionally pass nil to the :model argument, though I'd love to hear a reason from someone.

I think the PR is great but the functionality would only apply to form_for.

Thanks! I'm glad you like the PR. I don't know that I agree with you on moving it to form_for though, especially if as you thought, form_with was going to be a replacement plus you can freely use both methods and form_for just calls form_with eventually.

[mattpolito]

@cjilbert504 I just mean not passing model to form_with is a valid use-case. However, after reviewing the code again I see the distinction you made between the false and the nil check.

[cjilbert504]

@mattpolito - It definitely is a valid use-case which is why I didn't want to break that existing functionality. Thanks for chiming in on this one!

[p8]

The :model argument is supposed to be a model object, so this change makes sense to me:

rails/actionview/lib/action_view/helpers/form_helper.rb

Lines 602 to 603 in 8c5ebdc

	# * <tt>:model</tt> - A model object to infer the <tt>:url</tt> and
	# <tt>:scope</tt> by, plus fill out input field values.

This also makes form_with inline with form_for, which also raises if the model is nil.

rails/actionview/lib/action_view/helpers/form_helper.rb

Line 445 in 8c5ebdc

raise ArgumentError, "First argument in form cannot contain nil or be empty" unless object

[jhawthorn]

This PR changes the default value of the model: argument in form_with to false instead of nil and raises an ArgumentError in the event that the argument is nil.

This sounds like a breaking change, which we try to guarantee to users that they will see one minor version with a deprecation notice. Could we change this ArgumentError to a deprecation warning for Rails 7.2 and only raise the argument error in the following version?

[cjilbert504]

@jhawthorn - If that's what you all would like to happen here, I'm happy to do so. If that's the case, I will try to get to it ASAP. Thanks for the feedback/thoughts here!

[rafaelfranca]

Yes. Let's deprecate it.

[cjilbert504]

Copy that! I think I'll have some time on Monday or Tuesday that I can take care of it if that's cool.

[ghiculescu]

FYI @cjilbert504 an example of where this would break would be if you have a custom form helper that calls form_with internally, it will start raising as of this PR:

  # Renders a Rails form, enhanced with Turbo attributes. Specifically:
  # - Connects the form to the Form stimulus controller.
  # - Calls Form#submitStart when the form is submitted.
  # - Targets the "_top" turbo frame (ie. the full page) if no frame is provided.
  #
  # Accepts the same arguments as +form_with+: https://api.rubyonrails.org/classes/ActionView/Helpers/FormHelper.html#method-i-form_with
  # You can provide Stimulus options using the `stimulus` keyword. See the +stimulus+ method for options.
  def turbo_form_with(model: nil, scope: nil, url: nil, format: nil, stimulus: {}, turbo_frame: nil, **options, &block)
     form_with(model: model, scope: scope, url: url, format: format, builder: TurboFormBuilder, **form_options, &block)
  end

The fix is easy, but we should warn before we raise.

diff --git a/app/helpers/turbo_view_helper.rb b/app/helpers/turbo_view_helper.rb
index 123b64f6458..c413875ad29 100644
   # Accepts the same arguments as +form_with+: https://api.rubyonrails.org/classes/ActionView/Helpers/FormHelper.html#method-i-form_with
   # You can provide Stimulus options using the `stimulus` keyword. See the +stimulus+ method for options.
-  def turbo_form_with(model: nil, scope: nil, url: nil, format: nil, stimulus: {}, turbo_frame: nil, **options, &block)
+  def turbo_form_with(model: false, scope: nil, url: nil, format: nil, stimulus: {}, turbo_frame: nil, **options, &block)
      form_with(model: model, scope: scope, url: url, format: format, builder: TurboFormBuilder, **form_options, &block)
    end

[cjilbert504]

@ghiculescu - Yep, I understand that it would break as of this PR. I'm going to work on deprecating first as soon as I can. Thanks a bunch!

[cjilbert504]

@jhawthorn @rafaelfranca - Sorry I'm just getting back to wrapping this up (was sick and had power outages due to bad weather). Anyway, I wanted to ask about wrapping this up, would you all like me to make a new branch/PR to undo this and swap to the deprecation warning or just use this same branch and undo and swap to the deprecation warning?

[rafaelfranca]

It would need to be a new branch/PR.

[cjilbert504]

Perfect, because that’s what I did already 🤣. But wanted to confirm first. Thanks, @rafaelfranca

[cjilbert504]

@p8 @jhawthorn @rafaelfranca - I opened #50931 to resolve the issue of this PR introducing breaking changes. Let me know if I need to change anything about it and I will happily do so. Thank you all very much!