Move `convert_to_model` call from `form_for` into `form_with` #44468

seanpdoyle · 2022-02-17T20:58:10Z

Summary

Ensure models passed to form_with attempt to call to_model.

Now that form_for is implemented in terms of form_with, this commit
also removes the convert_to_model call from the form_for implementation.

To exercise this behavior, change existing form_with test coverage.

Prior to this change, a call to form_with made with a model: argument
that responds to to_model would not incorporate the instance's persistence
state into the form's HTTP verb. After this change, the persistence state
inferred from the model: argument's to_model call is incorporated into
the <form> element's [method] attribute.

This is a separate follow-up change proposed in rails/rails#44328.
The original change to restore old behavior deliberately excluded
applying the same logic to form_with, since it would be a breaking
change from how form_with behaved previously.

This commit proposed making that breaking change.

seanpdoyle · 2022-02-17T20:59:33Z

~~This is cut off of #44328, and includes its first commit (44eaa25).~~

~~When reviewing this branch's changes, skip to the second commit.~~

This has been rebased.

etiennebarrie

Can you describe a bit the breaking change?

If I understand correctly, an object needs to implements to_model to cause a change in behavior. If it does implement it and returns self like described in the docs, there's no change with this PR.
If the object implements to_model, but points to another object that conforms to the Active Model API, the form is broken in the sense that it will use the object for the action attribute, but the form's method will use the passed object persisted? (allowing for objects that don't implement it). So you can have an object that has a to_model that points to an active model that's persisted, but have a form with a POST because the object passed in doesn't implement it.

So it feels more like a bugfix than a breaking change? Maybe I'm missing some use cases?

actionview/lib/action_view/helpers/form_helper.rb

etiennebarrie · 2022-05-18T14:12:33Z

actionview/lib/action_view/helpers/form_helper.rb

@@ -438,7 +438,7 @@ def form_for(record, options = {}, &block)
          model       = nil
          object_name = record
        else
-          model       = convert_to_model(record)
+          model       = record


We don't really need model anymore, we could pass record to form_with directly.

I was hoping to avoid mixing too many refactoring changes with changes to the behavior. Inlining apply_form_for_options! feels reasonable, but might even be better suited as a follow-up PR.

While the parallel declarations might be confusing, it's a pattern that pre-dates this change so I felt like it was worth sticking with.

If it's decided that this isn't a breaking change, and that it's a worthwhile bugfix, I'd feel more comfortable broadening the scope to include some variable clean up. What do you think?

etiennebarrie · 2022-05-24T14:52:39Z

cc @gmcgibbon @byroot I think this is reasonable, I have a hard time finding an incompatibility that this would cause. If you implement to_model today it's only called in some places, e.g. the submit button will use the right wording (update vs create), but the action will be of the object passed to form_with, not the one from to_model. But if you implement the Active Model API like persisted?, but with a to_model that points to another object, it's kind of weird?

Also weird is that since we will call to_model on the result of to_model (in tag helpers like submit), if that points somewhere else that will be incompatible. But that sounds really like a weird edge case.

byroot · 2022-05-24T16:24:44Z

I like the general idea, but I'm a bit confused as I don't see any call to to_model in that diff.

Also such new feature needs a CHANGELOG.

seanpdoyle · 2022-05-24T16:33:31Z

I like the general idea, but I'm a bit confused as I don't see any call to to_model in that diff.

@byroot In order to shrink the diff back to its original size, I've reverted the refactor commit. With those incidental changes reverted, the focus of the diff involves the timing of calls to convert_to_model, which is where the call to to_model occurs.

Also such new feature needs a CHANGELOG.

The original change this is a follow-up to included a CHANGELOG entry:

Ensure models passed to form_for attempt to call to_model.

This PR was split in response to #44328 (comment), though I'm not quite sure how to most clearly characterize the (breaking) change into a new CHANGELOG entry.

byroot · 2022-05-24T16:35:12Z

@seanpdoyle ah, I see. Can you rework the PR title and description then, cause it's very hard to review in its current form.

etiennebarrie · 2022-05-24T16:36:33Z

It's the convert_to_model call in form_with.

seanpdoyle · 2022-05-24T16:46:30Z

@byroot I've renamed the PR title and tried to distill the change into the PR description.

gmcgibbon

Because this changes the behaviour of form_with, I think we should still add a changelog. Besides that this LGTM 👍

Ensure models passed to `form_with` attempt to call `to_model`. Now that `form_for` is implemented in terms of `form_with`, this commit also removes the `convert_to_model` call from the `form_for` implementation. To exercise this behavior, change existing `form_with` test coverage. Prior to this change, a call to `form_with` made with a `model:` argument that responds to `to_model` would not incorporate the instance's persistence state into the form's HTTP verb. After this change, the persistence state inferred from the `model:` argument's `to_model` call is incorporated into the `<form>` element's `[method]` attribute. This is a separate follow-up change proposed in [rails#44328][]. The original change to restore old behavior _deliberately_ excluded applying the same logic to `form_with`, since it would be a breaking change from how `form_with` behaved previously. This commit proposed making that breaking change. [rails#44328]: rails#44328 (comment)

seanpdoyle · 2022-05-25T13:49:52Z

@gmcgibbon I've added a CHANGELOG entry.

Upgrading to Rails 7.1 causes the RejectionReasons model to be passed as the `object_name` when rendering errors. This model doesn't hold any errors when it's passed to the form builder. This is because `form_with` now calls `#to_model` on the object passed to the form whereas it didn't in Rails 7. It's better that we don't depend on the internals of Rails for this feature. **Rails 7..7.1 fixes for `#to_model`** rails/rails#43421 rails/rails#44326 rails/rails#44468

rails-bot bot added the actionview label Feb 17, 2022

seanpdoyle changed the title ~~Form with to model method~~ form_with: Attempt to call to_model on model: Feb 17, 2022

seanpdoyle mentioned this pull request Feb 17, 2022

form_for: Attempt to call to_model on first argument #44328

Merged

seanpdoyle force-pushed the form-with-to-model-method branch 2 times, most recently from c337d32 to ea1ea3f Compare February 17, 2022 21:27

etiennebarrie reviewed May 18, 2022

View reviewed changes

seanpdoyle force-pushed the form-with-to-model-method branch 2 times, most recently from 677b5fd to 24321f0 Compare May 18, 2022 18:18

seanpdoyle force-pushed the form-with-to-model-method branch from 24321f0 to 95fd64e Compare May 24, 2022 16:27

seanpdoyle changed the title ~~form_with: Attempt to call to_model on model:~~ Move convert_to_model call from form_for into form_with May 24, 2022

gmcgibbon reviewed May 25, 2022

View reviewed changes

seanpdoyle force-pushed the form-with-to-model-method branch from 95fd64e to 980de46 Compare May 25, 2022 13:49

byroot merged commit 3404606 into rails:main May 25, 2022

seanpdoyle deleted the form-with-to-model-method branch May 25, 2022 14:27

ghiculescu mentioned this pull request Jul 24, 2023

Behavior changed when passing an array to form_for in Rails 7.0 #48789

Closed

inulty-dfe mentioned this pull request Mar 12, 2024

Upgrade Rails to v7.1.3.2 DFE-Digital/apply-for-teacher-training#9190

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move `convert_to_model` call from `form_for` into `form_with` #44468

Move `convert_to_model` call from `form_for` into `form_with` #44468

seanpdoyle commented Feb 17, 2022 •

edited

seanpdoyle commented Feb 17, 2022 •

edited

etiennebarrie left a comment

etiennebarrie May 18, 2022

seanpdoyle May 18, 2022

etiennebarrie commented May 24, 2022

byroot commented May 24, 2022

seanpdoyle commented May 24, 2022

byroot commented May 24, 2022

etiennebarrie commented May 24, 2022

seanpdoyle commented May 24, 2022

gmcgibbon left a comment •

edited

seanpdoyle commented May 25, 2022

Move convert_to_model call from form_for into form_with #44468

Move convert_to_model call from form_for into form_with #44468

Conversation

seanpdoyle commented Feb 17, 2022 • edited

Summary

seanpdoyle commented Feb 17, 2022 • edited

etiennebarrie left a comment

Choose a reason for hiding this comment

etiennebarrie May 18, 2022

Choose a reason for hiding this comment

seanpdoyle May 18, 2022

Choose a reason for hiding this comment

etiennebarrie commented May 24, 2022

byroot commented May 24, 2022

seanpdoyle commented May 24, 2022

byroot commented May 24, 2022

etiennebarrie commented May 24, 2022

seanpdoyle commented May 24, 2022

gmcgibbon left a comment • edited

Choose a reason for hiding this comment

seanpdoyle commented May 25, 2022

Move `convert_to_model` call from `form_for` into `form_with` #44468

Move `convert_to_model` call from `form_for` into `form_with` #44468

seanpdoyle commented Feb 17, 2022 •

edited

seanpdoyle commented Feb 17, 2022 •

edited

gmcgibbon left a comment •

edited