Question about form#save behavior with Rails about reform

[apotonick]

Hi Bryce,

it depends on the context you're working in: reform is an ORM-agnostic framework, so it doesn't know anything about the underlying database. That's why it pushes input data to the attributes using public setters.

In an ActiveRecord environment, however, model.save is called for you automatically. Are you working in an AR context?

[brycesenz]

I am working in an ActiveRecord environment, yes. But I don't like the idea of model.save being automatically called; all I want a form object to do is allow me to map my user interface to my domain models. Why should the form decide for me that I want to persist those models to the database?

Take the case where the workflow is

1. Get user input
2. Map input into three models, and pass those to a service that runs some decisioning logic
3. Based on that logic, decide to persist none/some/all of them to our database
4. Tell our controller how to respond.

That might be fairly uncommon, but it illustrates why my form shouldn't be calling functions that deal with persistence. Is there another method that ONLY maps the form input to a model, and returns and un-persisted AR model?

[apotonick]

The basic form without the ActiveRecord module mixed in does just that. Maybe we shouldn't mix in the ActiveRecord modules automatically. Have a look at the link I posted in my last comment, it shows what happens on top of the basic form in AR.

[brycesenz]

I understand the link you posted in your last comment. My point is that I question WHY you would do that. You're essentially saying that Reform is now going to try AR model objects and non-AR model objects differently. That doesn't make a ton of sense to me.

If the point of Reform is to undo some of the coupling that accepts_nested_attributes introduces, why are you introducing additional coupling? To illustrate, say ActiveRecord changes the name of its save function to be called "commit". Now you have to change your gem? That doesn't feel very clean, but it's just my two cents.

[apotonick]

I see your point. The basic reform gem is totally decoupled from AR, it doesn't know anything about it. Now, when you include the Form::ActiveRecord module, you explicitly say that want the coupling to AR - it is totally ok to have dependencies in this optional layer.

The only problem is that this "optional" layer is automatically included in Rails :) Should we introduce a switch?

[brycesenz]

I see your point as well. I mean, I can definitely see why someone might want it as a feature; I just personally don't like the idea of my forms handling persistence as well.

My argument would be that if you want to to automatically save to AR in Rails, you should have to optionally specify that. Or at least, there should be an option to prevent it. I'm curious what others think though.

[apotonick]

We're on the same page here - I hate this feature and introduced it only some days ago as lots of users were confused why reform would not save the underlying model. That's exactly why I have this in an optional module.

The more I think about this the more I agree we should have this configurable.

config.reform.save_model = true

[thomasritz]

How about naming it @form.sync instead of @form.save? The confusion is solely based on the name save. It misleadingly suggests a behavior that's not there.

So in my understanding the usage in the controller would be

if @form.validate(params[:song])
  @form.sync
  @song.save
end

[brycesenz]

That makes a lot of sense to me. Though for context, I'm new to using reform, so changing the naming convention doesn't really hurt my workflow much. Others might be upset about it. But yeah, I think that name is definitely more accurate of a description of what is happening.

[apotonick]

This boils down to aliasing #save to #sync. We could then document that sync in combination with the Form::ActiveRecord feature internally calls save on the model.

[brycesenz]

@apotonick - Hrm, that's different from where I thought that this was going. My understanding of @thomasritz 's suggestion was that the form support two methods - #save and #sync. #save would call save on the underlying models (assuming either a Rails environment or the inclusion of Form::ActiveRecord), while #sync would applying the form values to the underlying models while not calling save.

I see value in that implementation, but that's just my two cents.

[apotonick]

Since #save is already part of the public API we'd have to alias it to #sync anyway to make it consistent.

Following your proposal, #save internally had to call #sync and then save on the model (the latter only in an ActiveRecord/DM/.. environment).

I think an amended README could clarify the "syncing" behaviour of the #save method. Adding another public method might add more confusion than we already have. What do you think?

[apotonick]

I added some docs: https://github.com/apotonick/reform#saving-forms

[apotonick]

All at sudden I got it! You wanna sync but not auto-save your data in an AR environment, right?

[brycesenz]

Yes, that's exactly it! Syncing would just be coercing the form data into models without trying to save it to the database. So #save could remain the documented feature that it is today, while #sync would have this new behavior.