simply_helpful render(:partial) bugs

I've got a couple of tickets filed for simply_helpful dealing with render(:partial) ([1], [2]); does anybody have any thoughts? Am I misunderstanding the simply_helpful code, or are these real?

It'd be nice to see these get checked in (or at least reviewed) if they are actual bugs.

Nick

[1] Ticket 6719: http://dev.rubyonrails.org/ticket/6719 [2] Ticket 8410: http://dev.rubyonrails.org/ticket/8410

It'd be nice to see these get checked in (or at least reviewed) if they are actual bugs.

Hi Nick,

Simply Helpful has been folded into core. Could you check if these problems are still present on edge? If so, please do post again here and I'll apply.

Doh! When did that happen? Last time I checked it was still waiting to be folded.

Yes, the core seems to have the same problems. I'll refile the bugs as soon as Trac is back up (I'm getting a 'duplicate key violates unique constraint "node_change_rev_key" ' error).

For now, here are the problems I see (this is all assuming you're using record identification):

For reference, here's how the variables are passed:

in action_view/base.rb, line 308:    render_partial is called with the object, wrapped nil, locals

which means that in partials.rb, line 50:     partial_path is the object, local_assigns is wrapped nil, deprecated_local_assigns is the locals hash

Here are the problems: 1. when the object is an Array (i.e., we're rendering a collection) (line 67):     we sensibly name path and collection     we call render_partial_collection with the correct path and collection, but we use local_assigns.value, which is nil (see above).

Thus locals are not distributed over collections when using identification.

2. when the object is not an array (i.e., we're rendering a single object) (line 75):     we call render_partial with the path, local_assigns as the object (which is a wrapped nil), and deprecated_local_assigns as the locals hash (which is the actual locals hash)

Thus the object is not properly forwarded to render_partial(string ...).

Also, for the collection, the test for a trivial collection is inconsistent with the assumptions made: if we test for any?, then we should use the first non-nil value, instead of just the first.

Here's a suggested patch:

http://pastie.caboo.se/63352

Nick

ObjectWrapper, not WrapperObject.

http://pastie.caboo.se/63368

Trac seems to be back up; ticket 8426 ( http://dev.rubyonrails.org/ticket/8426 ).

Nick

Trac seems to be back up; ticket 8426 (http://dev.rubyonrails.org/ticket/8426

Good stuff. Could you also add some tests that verify the behavior? We're trying to enforce that on all new patches. Thanks!

Not sure if my last message made it through, so I'm reposting.

I came up with tests; one of them still fails, though: the collection- containing-nil one. I'm not really sure what to do with that; anyone have any thoughts on what should happen if you try to render :partial => nil ? What about render :partial => [nil, a_model] ? My tests expect that render :partial => [nil, a_model] is the same as render :partial => a_model

Nick

I came up with tests; one of them still fails, though: the collection- containing-nil one. I'm not really sure what to do with that; anyone have any thoughts on what should happen if you try to render :partial => nil ? What about render :partial => [nil, a_model] ? My tests expect that render :partial => [nil, a_model] is the same as render :partial => a_model

I like just ignoring nil. As in, let it call .compact on the collection before starting to work with it. Be sure to add this to the docs, though.

Sounds good; I'm uploading a new patch. All the tests I wrote pass now, and I added a doc note.

One last thing: I'm wondering if we should add a case for AssociationCollections, so that you can have

class CatOwner    has_many :cats end

render(:partial => @cat_owner.cats)

It might look like

when ActiveRecord::Associations::AssociationCollection   render_partial(partial_path.to_a, local_assigns, deprecated_local_assigns)

This doesn't happen automagically, I don't think...

Thoughts?

Nick

One last thing: I'm wondering if we should add a case for AssociationCollections, so that you can have

class CatOwner    has_many :cats end

render(:partial => @cat_owner.cats)

That already works. When we get an array, we just call render :partial for each of the members and join it. Don't want AP to know anything explicitly about AR. We could be collecting ARes or other model objects and it should work just the same.