Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

exception raised when nested property is nil #67

Closed
scharfie opened this issue Feb 5, 2014 · 10 comments
Closed

exception raised when nested property is nil #67

scharfie opened this issue Feb 5, 2014 · 10 comments

Comments

@scharfie
Copy link
Contributor

scharfie commented Feb 5, 2014

The following code for me is triggering a big stacktrace with error NoMethodError: undefined method name' for nil:NilClass`

class Album < OpenStruct
end

class AlbumForm < Reform::Form
  property :artist do
    property :name
  end
end

AlbumForm.new(Album.new)

One way around this is to override the #initialize and ensure that the nested property (model.artist) exists, but I think that might be too brittle. I have a couple of ideas on how to possibly fix this:

  1. #initialize could call a new prepare_model method which subclasses of Reform::Form could override. Not a great deal better than overriding #initialize though.

  2. property method could support a :builder option that takes a block, or symbol containing a method name to invoke on the model (i.e. :build_artist in a has_one/belongs_to ActiveRecord relationship).

What are your thoughts?

@adamalbrecht
Copy link

+1

I just ran into this issue as well for my has_one association.

@apotonick
Copy link
Member

This is a common misunderstanding of how Reform and representable work with models. This discussion gives some insight. Basically, it's up to you to setup your model before passing it to the form.

However, since this seems to be pretty inconvenient, representable will come up with a solution.

@mattheworiordan
Copy link
Contributor

@apotonick, I too have this issue. The problem for me is that with the example above, it is completely valid to have a nil value for artist, so I need Reform expect either a model or a nil object. I am really not clear how I should deal with this problem unless I perhaps define Null objects and inject them in when the form is set up.

@apotonick
Copy link
Member

Let's say reform would allow nil properties at initialization (e.g. artist being empty), how do you handle that when rendering the form? Do you do stuff like

- if @form.artist 
  = render 'form/artist'

or how do you guys deal with those edge-cases?

@apotonick
Copy link
Member

Fixed in master: 0dbbe0c#diff-55fdd0dd35969b77c2c643869ac9d25eR182

Please test and also I need some feedback on my last comment.

@mattheworiordan
Copy link
Contributor

@apotonick, thanks for this, I will test it out.

However, as you say, I suppose importantly we need to consider what will happen with Reform::Form if the underlying model is empty. Any call to save would clearly blow up, but I think that is fine. I assume all the validations and setting of attributes via validate will continue to work?

BTW. On a separate note, I originally started using another form gem, but quickly switched across to Reform as I felt like I was adding too much for it to be viable. One feature I added, which is often needed for me, is to use a form object not backed by any model, see GCorbel/activeform-rails@a98fd5b for the commit that adds this functionality. I think implementing your fix above could easily be modified to support this behaviour as well, see what you think.

I'll issue a PR now to cover expected behaviour. I don't claim this is right, it's just what I think I would need.

@apotonick
Copy link
Member

I have some thoughts on form objects not backed by any model: I call this a "contract" or "Validator" in an upcoming pattern collection. Reform can handle that out-of-the-box. What's your intended workflow with such a form object?

@mattheworiordan
Copy link
Contributor

Here's an example where I am using a custom wrapped form object. Does that make sense?

How would Reform handle that out of the box?

@apotonick
Copy link
Member

You'd just do AlbumForm.new(OpenStruct.new) and then do a form.save do |f, hash| where hash is what you want.

Maybe this is a good thing to document. And the reason this might look clumsy is because Reform explicitely tries not to expose 721 public methods but a clean set of as-stateless-as-possible public concerns.

@mattheworiordan
Copy link
Contributor

Mmm, as you will see in the tests I provided, simply providing an OpenStruct.new will not work :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants