trailblazer / reform

Form objects decoupled from models.
https://trailblazer.to/2.1/docs/reform.html
MIT License
2.5k stars 184 forks source link

Populator does not run if attribute is not present in validation params. #335

Closed tmullen closed 8 years ago

tmullen commented 8 years ago

I noticed a strange problem with populators on scalar properties and when they get run. Looks like a property's populator is only run if the params passed to the validator contain a key/value pair for that property.

Here's a quick example to demonstrate the issue:

class PopulatorTest < MiniTest::Spec
  Album = Struct.new(:name)

  class AlbumForm < Reform::Form
    property :name, populator: ->(options) { self.name = "populated!" }
  end

  let (:album) { Album.new }
  let (:form) { AlbumForm.new(album) }

  it "works" do
    form.validate({name: "empty"})
    form.name.must_equal "populated!"
  end

  it "doesn't work" do
    form.validate({})
    form.name.must_equal "populated!"  #=> form.name is nil
  end
end
apotonick commented 8 years ago

Yeah, that's per design! A populator is an instruction to run specific population logic for a particular fragment.

You can run always run it with this "trick".

property :name,
        deserializer: { parse_pipeline: ->(*) { ->(input, represented:, **) {
        represented.name= ... }} }

I will soon abstract that into Reform.

tmullen commented 8 years ago

Oh, interesting. I'd thought the populator logic was always run, and not conditional on the fragment. Thanks for setting me straight. :)

apotonick commented 8 years ago

Ha, no, I can add that to the docs. You should also consider reading the Nested Forms and Mastering Forms chapters of the Trailblazer book</plug>...!

apotonick commented 8 years ago

Check the second paragraph: http://trailblazer.to/gems/reform/populator.html#populator Does that help? Thanks for pointing that out!

tmullen commented 8 years ago

I think when I was scanning the documentation, I came across this:

While the :populate_if_empty option is only called when no matching form was found for the input, the :populator option is always invoked and gives you maximum flexibility for population.

That's what threw me....though now I understand the context in which you were writing. If I'm the only one, then this addition is probably fine where it is. Otherwise, I'd try to fit it into the general description at the top somehow. My two cents anyway.

Thanks for the help.

jonathanpa commented 8 years ago

@apotonick

I will soon abstract that into Reform.

Did have time to code it or should I still use deserializer: { parse_pipeline: ->(*) { ->(input, represented:, **) ... ?

apotonick commented 8 years ago

No time, yet! :tired_face:

dnd commented 7 years ago

@apotonick in your workaround you use a deserializer. In your opinion is using a deserializer the right way forward, or should the ultimate answer be some type of populator? Maybe changing populator to always eval, or having a populate_always option that is mutually exclusive with populator and populate_if_empty?

Changing populator to always execute would likely be a breaking change, as people would have to update their populators to deal with cases where no data was passed I think, right?