Rails Ruby on Rails → 2.x

Comments and changes to this ticket

• You flagged this item as spam.
  

  David Yip March 8th, 2009 @ 11:02 AM

  Here's a manual test in the form of a small Rails application that exercises the full stack in a scenario similar to the one in the problem description. To exercise it, go to /posts.

  • 2172-nested-model-test-app.tar.gz 76 KB

• You flagged this item as spam.
  

  David Yip March 8th, 2009 @ 11:05 AM

  • Title changed from “Nested model forms error on loading data from one-to-one associations that evaluate to nil” to “Nested model forms error on loading data from belongs_to associations that evaluate to nil”
• 

  Eloy Duran March 8th, 2009 @ 01:03 PM

  I agree we should fix this error people run into. But I wonder if simply using a regular fields_for is what the user expects. For instance, how about dynamic defaults that might be set when a model is first instantiated?

  Rather I would opt for not yielding a form builder at all if the association is nil. This would be more consequent with the collection variant as well. As in, if there's no associated record then don't yield a new form builder.

  So in this case the user knows clearly that he/she should initialize an instance in order to get the form builder, but a user can also choose not to have a form builder if there's no association. Thus leaving the control in the controller where it should be.

  
  class PostsController < ApplicationController
    ...
    def new
      @post = Post.new
      @post.build_author
    end
    ...
  end
  

  Thoughts?

• You flagged this item as spam.
  

  David Yip March 8th, 2009 @ 08:06 PM

  Hmm, let me make sure I understand the proposal.

  In this case, if post.author is nil, then the render result for

  
  <% form_for @post do |f| %>
    <% f.fields_for :author do |a| %>
      <%= a.label :name, 'Author name' %>
      <%= a.text_field :name %>
    <% end %>
    <%= f.submit %>
  <% end %>
  

  would be equivalent to the render result of

  
  <% form_for @post do |f| %>
    <%= f.submit %>
  <% end %>
  

  ?

  I think this would be fine, provided (of course) that such behavior is documented on fields_for.
  

  It does make controller code for complex nested model schemas a bit more tedious to produce; I'm currently working on an application that would end up looking like

  
  class PostsController < ApplicationController
    def new
     @post = Post.new
     @post.build_author
     @post.author.build_foo
     @post.author.build_bar
     ...
  end
  

  But I don't think this extra verbosity is necessarily a problem. As a counterpoint, though, there was a comment on #1930 by pete_lacey (http://rails.lighthouseapp.com/p...) in which he mentioned "27 deeply nested calls to accepts_nested_attributes_for", and I can see how this could get troublesome. I'll let others who are actually going that far chime in on that.

• 

  Eloy Duran March 8th, 2009 @ 10:08 PM

  Yes, you got it right. And it should definitely be mentioned in the documentation.

  About the extra work necessary in the controller; you might want to shove that into the Post model if a Post should have those associations anyway.

  
  class Post < ActiveRecord::Base
    after_initialize :build_author
  end
  
  class Author < ActiveRecord::Base
    after_initialize :build_foo
    after_initialize :build_bar
  end
  
  class PostsController < ApplicationController
    def new
     @post = Post.new
     ...
  end
  

  Obviously if you only use these models in one controller it won't clean up the amount of lines of code. But the controller or model are more appropriate places for this so one has more control. Especially in those cases like Pete's, the model is definitely where you'd want to solve this imo.

• You flagged this item as spam.
  

  David Yip March 9th, 2009 @ 04:10 AM

  Yeah, there's definitely times where the model is a more appropriate location for this sort of initialization code.

  Here's another patch that implements the "don't yield a builder" behavior. It also adds some documentation for that behavior. Returning an empty string from fields_for_with_nested_attributes feels dirty for some reason I can't qualify, but it does communicate the intent and passes all Action Pack tests, so I'll leave elegance to those who are more familiar with this part of the Rails codebase :)

  • 0001-Don-t-yield-a-form-builder-for-nil-one-to-one-associ.patch 3 KB
• 

  Eloy Duran March 9th, 2009 @ 10:02 AM

  • Assigned user changed from “Eloy Duran” to “Michael Koziarski”
  • State changed from “new” to “open”

  Very nice, thanks a lot!

  Returning an empty string from fields_for_with_nested_attributes feels dirty for some reason I can't qualify, but it does communicate the intent and passes all Action Pack tests, so I'll leave elegance to those who are more familiar with this part of the Rails codebase :)

  I have refactored it a little bit, which now also makes it return nil instead of an empty string, your test still passes so it should be good enough.

  I also moved the test down in the file to where the other tests related to nested attributes are and renamed the method to match that of the others.

  +1

• 

  Eloy Duran March 9th, 2009 @ 10:02 AM

  • no changes were found...

  • do_not_yield_form_builder_for_nil_one_to_one_association.diff 6.1 KB
• 

  Pratik March 9th, 2009 @ 11:28 AM

  not sure I like the idea of silently eating the f.fields_for block when the association is nil.

• 

  José Valim March 9th, 2009 @ 11:42 AM

  I agree with Pratik. Why not see if the :author is an association on Post and call build_author?

• 

  Eloy Duran March 9th, 2009 @ 12:47 PM

  not sure I like the idea of silently eating the f.fields_for block when the association is nil.

  Like discussed in #rails-contrib, this is not silently eating it (there is even extra documentation about it). See my earlier comment:

  Rather I would opt for not yielding a form builder at all if the association is nil. This would be more consequent with the collection variant as well. As in, if there's no associated record then don't yield a new form builder. So in this case the user knows clearly that he/she should initialize an instance in order to get the form builder, but a user can also choose not to have a form builder if there's no association. Thus leaving the control in the controller where it should be.

• 

  Eloy Duran March 9th, 2009 @ 12:51 PM

  I agree with Pratik. Why not see if the :author is an association on Post and call build_author?

  First of all because, of the reason I mentioned earlier; it's inconsistent with collection associations.

  And second, it would mean that we would have to add a bunch of AR specific code into the form helper, which is not something I'd like to do. Right now only new_record? is being used, which works on DM as well.

• 

  Pratik March 9th, 2009 @ 12:55 PM

  I wouldn't really worry about DM. That should be a separate concern altogether.

• 

  Eloy Duran March 9th, 2009 @ 01:39 PM

  After discussing this with Manfred, we came to the conclusion that it might be best to raise informative exceptions. Because I personally, and others with big forms, haven't needed this kind of magic yet, so I don't feel strongly about it. What I do feel though, is that building records is not a concern of the view.

  Anyways, currently form_for only fails with the default whiny_nil message. So we were thinking exceptions along the lines of:

  • "ArgumentError: The object found by or passed to form_for is nil."
  • "ArgumentError: The object found by or passed to fields_for is nil."
  • "ArgumentError: The object found by or passed to fields_for is nil, if you're rendering a form for a nested model make sure you build one first."

  Thoughts?

• You flagged this item as spam.
  

  David Yip March 9th, 2009 @ 04:02 PM

  I think exceptions would be fine; I personally haven't had a need for using forms on schemas that don't match what is expected by the form.

  However, form_for doesn't currently raise an exception it finds is nil; it only occurs in the explicit object case. For example, given @post = nil,

  
  <% form_for :post do |p| %>
    <%= p.text_field :title %>
  <% end %>
  

  currently renders a form with a text field having name post[title]. Is this behavior that should stay around, or should form_for be modified to require an object?

• 

  Eloy Duran March 9th, 2009 @ 08:14 PM

  currently renders a form with a text field having name post[title]. Is this behavior that should stay around, or should form_for be modified to require an object?

  Hmm, you're right. I personally don't really understand why that's supported at all, or have had a need for it, so let's see what Michael has to say about this.

• 

  José Valim March 19th, 2009 @ 11:53 AM

  What if we raise an error on fields_for just if the parent form object is not nil?

  That would cover both cases:

  1. It would work without objects (@post=nil and @author=nil).

  2. It would raise an error when @post is not nil but @post.author is nil, as proposed previously.

• You flagged this item as spam.
  

  Ivan Vanderbyl March 19th, 2009 @ 11:08 PM

  In my case this fixed the problem, simply not calling new_record? on nil and handling it as though it was an existing record.

  
  module ActionView
    module Helpers
      class FormBuilder
        def fields_for_nested_model(name, object, args, block)
          if object && object.new_record?
            @template.fields_for(name, object, *args, &block)
          else
            @template.fields_for(name, object, *args) do |builder|
              @template.concat builder.hidden_field(:id)
              block.call(builder)
            end
          end
        end
      end
    end
  end
  

• 

  Eloy Duran March 29th, 2009 @ 12:37 PM

  • Assigned user changed from “Michael Koziarski” to “Eloy Duran”

  Please see #2365 for a possible solution on building new associated records if they don't exist yet.

• 

  José Valim August 8th, 2009 @ 12:44 PM

  • State changed from “open” to “duplicate”

  Marking as duplicated. As Eloy said, try #2365 and let your comments there, so we can get it applied.

• 

  Ryan Bigg October 9th, 2010 @ 09:45 PM

  • Tag cleared.
  • Importance changed from “” to “Low”

  Automatic cleanup of spam.

• You flagged this item as spam.
  

  bingbing March 26th, 2011 @ 01:23 AM

  gucci watches