Why another form object?

[mattheworiordan]

@GCorbel, I was pretty excited when I saw your form object the other day, so integrated at the earliest opportunity. Thanks for contributing it to the community. However, as you can see, I've hit a number of issues using it and have done what I can to help by submitting my two pull requests #2 and #1.

I have now gone home and started wondering if I made the right choice changing from our home grown object models to this gem simply because it's turned out to be a lot more work than I thought. I then did a bit of searching too and came across https://github.com/apotonick/reform.

I'd like to know specifically what you were aiming to achieve with this Gem as opposed to simply using https://github.com/apotonick/reform? I am happy to help contribute, but equally if there is a gem out there that already does the job well, I'd like to know why we shouldn't just use that.

[GCorbel]

I know reform and I did some pull request. I made this gem because I tried reform and I found some bugs. I started to contribute but there is some things I don't like in reform.

First, I don't like than the #validate method is responsible to fill attributes for model.
Secondly, I really don't like this method.
Finally, I really do something simple and I find the reform's implementation is a little bit too complicated for what I want. I think my code (and yours) is simple.

What you think?

[TylerRick]

@GCorbel, when I was looking at reform and deciding whether to use it, I didn't like how the #validate method was responsible to fill attributes for model either. But when I dug deeper I discovered that it simply passes on the attributes hash to #from_hash. So you can always skip using validate and use from_hash instead, if that helps any...

I find reform's implementation a bit too complicated too (lots of layers of abstraction, including going through the representable gem for a lot of things), but it seems to be well-designed at least...

[mattheworiordan]

@GCorbel, I am not sure #validate updates the models, I thought you had to call #save to update the models?

@TylerRick, the reason I need to use `#validate' is because the primary reason I am using a form object is to centralise all logic around parsing params and validating those params in the form object. So the validations are essential from my perspective.

[GCorbel]

@TylerRick I already seen the #from_hash method.

@mattheworiordan Another problem I found with Reform is the synchronisation with models. The object you passed in argument to reform does not have the same value than the form.

If you compare the code of Reform and the code of ActiveForm-Rails, I think the last is more simple and clear for a behavior similar (or better).

[TylerRick]

@mattheworiordan, I think you're right. #from_hash (and therefore #validate, which calls it) only updates the form object from your params. So then you can access that data via form.songs or form.to_hash.

But you're right, I think the idea is that you don't want to "pollute" your model objects with that data unless validate is successful (returns true). #save then calls save_to_models which is what actually updates your model objects from the form data.

trailblazer/reform#43 (comment)

"… Reform does not sync input data to the model until you call form.save. …"
"… Don't let the data model know anything about what's going on until the data is validated, sane and eventually coerced."

@apotonick, I think the "save" semantics may be confusing to some people (including me). How about we rename it to sync_to_models to make it clearer that it only syncs to the model objects but doesn't also save them? (I suppose "save" may have different meanings, but at least as far as ActiveRecord is concerned, most people assume it means save to the database...) I would suggest removing #save from the main Form class (forcing people to call the more intention-revealing sync_to_models), but leaving it in the ActiveRecord integration (and any other integrations that actually persist the data to a database).

@mattheworiordan, I agree that the validations are essential most of the time. By the way, have you found a good way/pattern to have it validate using both the form-specific validations in your form class _and_ the validations in your underlying models? I guess in the simple case (if you're relying on the behavior of Form#save without a block) it might be something like this:

  if form.validate(params[:key]) and form.save
    redirect ...
  else
    render 'form'
  end

but I'm worried that it would become ugly if you tried to use the block form...

[GCorbel]

About #validate which fill attributes of the form, I think it's a problem of architecture and clarity. If you respect the Single Responsabilty Principle, you must to have two methods. The validate method do two thing really different. Furthermore the #from_hash is hidden and undocumented. We don't know exaclty what the #validate method do if we don't read the code.

[mattheworiordan]

@TylerRick, ha, I can't believe you just asked how we deal with the underlying model validations after the form validations. This is exactly the discussion we've just had in our team this afternoon when I presented my new work and refactoring I have done. We actually have 3 layers where validations may exist, here is some pseudo code:

form_object.validate(params) # first validation

model.attributes = form_object.to_hash # validation errors could occur here because not covered by the form object

my_service.update_model(form_object.to_hash) # this could fail for other reasons and would too have errors

I have developed a simple pattern that copies errors from the model or the service into the form object so that the view can still use those errors and the form object becomes ubiquitous, however it's a bit messy and very domain specific.

Do you think think it would be worth me writing up a simple Gem that could handle this all in a nice generic fashion? I think so...

[TylerRick]

Good points, @GCorbel, I completely agree. I like your API here much better than reform's API.

In your API, you do these as two separate steps:

form.fill_attributes(hash)
if form.valid?
  if form.save # save all models
    redirect_to …

whereas reform conflates these two responsibilities into a single validate method:

if form.validate(hash)
  if form.save
    redirect_to …

I also like how your API (#valid? and #save) is the same as ActiveRecord::Base and makes dealing with a form object almost the same as dealing with a model.

I would suggest you rename fill_attributes to assign_attributes (maybe aliased as attributes=) though, so that that is the same API that people are used to with their ActiveRecord models.

resource.assign_attributes(hash)
if resource.valid?
  resource.save
  redirect_to …

Also, I'm curious, does calling save also call valid? first, the same as an ActiveRecord model does? Then you could simply do this and skip the extra call to valid?:

form.assign_attributes(hash)
if form.save
  redirect_to …

I really like your API in ActiveForm. I may consider switching from Reform to ActiveForm...

@apotonick, what do you think of @GCorbel's comments about the single responsibility principle? Would it be all right if we completely got rid of #validate in Reform and required the use of separate #assign_attributes and #valid? methods instead?

[GCorbel]

@TylerRick if you check the implemention of the #save method you can see than it's return the result of #valid?.

I totaly agree with #assign_attributes and #attributes=.

I am also intersted by the opinion of @apotonick. He did a great work and his experience can be very useful.

[TylerRick]

@mattheworiordan, I think that would be very useful. I too was wondering how I would roll up all of the errors from my model objects back into the form object so that I can actually show the errors in the view next to the appropriate fields.

Do you think this should actually be solved in Reform? There is already a one-to-one mapping between fields in the form and the fields in your model objects. Reform can already sync values between the model object and form object (in both directions, I believe, using representable). So maybe we just need to add the piece that syncs errors from the model object to the form object.

I'm guessing this would probably belong in form/active_record.rb. Then we could change the default save behavior for ActiveRecord-backed forms be to save and if the save fails, merge its errors back into the form`s. Maybe something like this?

    def save(*)
      super.tap do
        return if block_given?
        model.save.tap do |result|
          unless result
            merge_errors_from(model)
          end
        end
      end
    end

(Also noticed this merge! method. Not sure if it's helpful or not.)

As for copying errors from the service into the form object, that probably wouldn't/couldn't be done in Reform, since it doesn't know anything about your service objects. So maybe you could still write a gem for that portion...?

[apotonick]

The #validate method in Reform does not touch the model unless ActiveModel's fancy validators do fancy stuff. It only updates the form's internal representation of the fields.

If you use #from_hash you're only updating the form representation. It's undocumented because it's private ;)

I agree, the #save might confuse people and it could be split up into #sync and #save.

The reason Reform does updating attributes and validation in the same step is because I wanna reduce public methods. This is to save users from having to remember state.

There are multiple tickets for this and as soon as I finished the representable 1.8 upgrade I will give Reform some more ❤️

API-wise, this gem here basically copies all the concepts found in Reform and puts some Rails sugar on top - I am not saying this is bad but I wonder why we don't incorporate that in Reform itself.

[GCorbel]

The reason why I created another gem was just because it was simpler and faster to do it. I can not wait to see the next version of reform. Thanks for your great job.

[apotonick]

I apologize for the slow development of Reform after the "explosion" when I released it initially. The reason for this is I changed jobs and didn't use Reform (yet).Thanks for all your patches and ideas so far!!!!!!!!!!!11one

[GCorbel]

No problem. I can understand. This is offtopic but why do you change your job because of a open-source project?

[apotonick]

Haha, I meant to say after I published Reform I changed jobs (unrelated to Reform) and in the new project, we don't use Reform, yet.

[mattheworiordan]

@TylerRick, glad to hear you think it could be useful. I was thinking a DSL that was not connected to any particular class, but would perhaps only expect that any model would respond to valid? and errors, so here is some pseudo code:

form.assign_attributes(hash)
form.sync_and_validate_with(form.model).then_validate do
  my_custom_service.update(form.model, form.attributes)
end

if !form.valid?
  # render view with form object and errors on the form
end

So the above would validate all objects passed into #sync_and_validate_with as the same time as validating the form. This is important to do at the same time because it's quite possible a validation error exists on the form object and the underlying model, and if we do these together we can present all validations in one form object.

The #then_validate will only be executed if form & objects are #valid?. It would then execute the black and store the result. If the result responds to #valid? and provides #errors then again these errors can be added to the form object.

My only concern with this approach is that if someone calls #valid? on the form object afterwards, it would under the hood currently delete the existing errors on the form object and revalidate. The could have unexpected side effects where the errors added by the models passed in or the service called will be lost.

As such, I'd be inclined to change the behaviour of the predicate #valid? to never change the object, and instead only revalidate the form object when validate is called. I suspect this would introduce some issues though unfortunately.

What are your thoughts on this?

[GCorbel]

@mattheworiordan #then_validate does not respect the law of demeter. I really fill like Brainysmurf.

What if there is a form with multiple models. How do you manage this?

[mattheworiordan]

@GCorbel, what do you mean it does not respect the law of demeter. then_validate would only expect the return value of the object passed to it and would expect it to adhere to an interface which has ActiveModel like #valid? and #errors.

I kept it simple intentionally, but for a Form with multiple models and multiple return values, you could easily modify as follows:

form.assign_attributes(hash)
form.sync_and_validate_with(form.person, form.place, form.company).then_validate do
  result = my_custom_service.update(form.model, form.place, form.company, form.attributes)
  [result, result.model, result.place, result.company] # pass in array of objects that have errors copied to the form
end

if !form.valid?
  # render view with form object and errors on the form
end

My concern with this approach is still that it's somewhat brittle with the current implementation of valid? because whilst valid? appears to be a predicate and should have no side effects, this is not the case and could remove the errors applied by one of the steps above.

[GCorbel]

@mattheworiordan there is two . in form.sync_and_validate_with(form.person, form.place, form.company).then_validate so the object do not talk only to his immediate friends.

Furthermore, you call #sync_and_validate_with and #then_validate. When I read this line I understand than it do the validation two times.

I don't like the fact you must to return an array and you probably must return it in the right order.

I agre with your concern. I realy prefer to do this :

form.assign_attributes(hash)
if form.valid?
  my_service.update(form)
  #render something
else
  #render somthing else
end

It looks more like a normal controller.

[mattheworiordan]

@GCorbel, form.sync_and_validate_with returns a class that is responsible for then_validate, so there is no crossing of concerns. It's like RSpec's expect(form.person).to then_validate.

In your code example, where are the models validated and how do the validations pass to the form object?

Secondly, if the service fails, how do the errors get presented to the user, especially if they are inline errors?

I am not really following your code example in the context of what we're discussing.

[GCorbel]

The model is passed on the creation of the form like this form = Form.new(model: model). In fact, I am describing what I do with ActiveForm-Rails.

Why a service would fail if the form is valid? If it fail, it will throw an error.

[apotonick]

I've been following this discussion with interest. The difference between the two gems is: Reform is a class with a minimized number of entry points (aka public methods), whereas you guys are designing a DSL.

DSLs can be problematic for the user since the user has to manage state (e.g. am I supposed to call valid? first or update_attributes?). This is exactly why the #validate is the only method to change state in Reform.

About #validate which fill attributes of the form, I think it's a problem of architecture and clarity. If you respect the Single Responsabilty Principle, you must to have two methods.

This is wrong. SRP means your class does exactly one thing, which is reflected in a single public method. The more methods you expose, the less SRP you go.

Trust me, I thought a lot about #validate and its semantics, and I am gonna make it even more "SRP" by making Form#errors and #valid? semi-public. All that happens via #validate reducing the possible wrong usage for users.

What was the original problem with that method in the first place?

[apotonick]

I updated the README - is it ok when I link to this discussion? Might be a bit off-topic....

[mattheworiordan]

@apotonick, fine by me to link here if you think it's useful.

@GCorbel, in response to your points, I am still not following in your example how validation errors of even problems on attributes can be passed back to the form without doing it explicitly in the controller code.

I am in no way suggesting ActiveForm-Rails or Reform should provide this DSL, I think it would most likely be wrong to bake this type of functionality into a form object, however I do think it's a common pattern that should be solved, and I am probably going to try and solve it as a Gem as opposed to simply writing code that we use in our code base.

Goal: In a controller, use a single consistent Form object in the view for any action

Example:

class AccountService
  include ActiveModel::Validations
  def self.create(account)
    errors.add(:email, 'Email does not exist on 3rd party') if 3rdPartyService.validate_email(account.email)
    account.save! if account.valid?
  end
end

class AccountController 
  def new
    @account = AccountForm.new(Account.new)
  end
  def create
    @account = AccountForm.new(Account.new)
    @account.update_attributes(params[:account])
    @account.sync_and_validate_with(:model).then {
       AccountService.create(@account.model)
    }.and_check_valid
    redirect_to account_path(@account) if @account.valid?
  end
end

What this solves

• The view can always reference the same Form object and access methods like @thing.valid? or @thing.errors.on(:email)
• It's far less error prone in case you don't include a validation in the form object that is actually need in the underlying model. This is better for maintainability because if the model validations change in future, the form won't blow up and not show why the model could not be saved. Also, if there is a complex rule on the model that is not practical or easy to replicate in the form object, the form object can gracefully handle validations on the underlying model and copy them up to the form object.
• If after validating the models & the form object validations pass, but there is still an error when creating the models using a service (such as a check to a 3rd party for validations or even a field no longer being unique), a consistent interface is provided to the view with the form object and errors can be copied to the form with the :base message or on particular fields if applicable.

I suspect we're not really going to agree on a style we both like. It's been a good debate and I think I'm still keen to solve the problems outlined above. I will when I have a chance see if I can knock something together.

[GCorbel]

@mattheworiordan, with ActiveForm-Rails, validations is the responsability of the form and not of the models. There is no need to synchronize errors from the form to the models and vice versa.

We have 3 methods to resolve the same problem. It's a very interessant debate with no final anwser. Like all best practices, I think the way you will resolve a problem will depend of the application you are doing.

I close the issue but we can continue the discussion.

[TylerRick]

I do think it's a common pattern that should be solved, and I am probably going to try and solve it as a Gem as opposed to simply writing code that we use in our code base.

@mattheworiordan Did you by any chance ever finish/publish your solution for this? (the code that provides sync_and_validate_with)

I'm currently looking for/at form object libraries and re-evaluating which one to use, and I resonated with most of the ideas you shared so would be interested in seeing what you use these days (if anything)...

I'd like to move away from Reform and towards something that more directly reuses standard ActiveModel modules... like https://github.com/oozou/active_form/blob/master/lib/active_form/base.rb , for instance