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.