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

ActionView::PartialRenderer #collection_with_template and #render_partial do not forward layout yield arguments to partial #12820

Closed
bughit opened this issue Nov 9, 2013 · 5 comments

Comments

@bughit
Copy link
Contributor

bughit commented Nov 9, 2013

the docs suggest they should
http://api.rubyonrails.org/classes/ActionView/PartialRenderer.html

    def collection_with_template
      view, locals, template = @view, @locals, @template
      as, counter = @variable, @variable_counter

      if layout = @options[:layout]
        layout = find_template(layout, @template_keys)
      end

      index = -1
      @collection.map do |object|
        locals[as]      = object
        locals[counter] = (index += 1)

        content = template.render(view, locals)
        content = layout.render(view, locals) { content } if layout
        content
      end
    end

the problematic lines AFAICT:

        content = template.render(view, locals)
        content = layout.render(view, locals) { content } if layout

the problems:

  1. the template is rendered before the layout and regardless of whether the layout actually yields
  2. due to Rails is not Django #1, it's too late to make use of yied args

should it not be something like this

layout.render(view, locals) { |**locals_from_layout|  template.render(view, locals.merge(locals_from_layout) }
@robin850
Copy link
Member

robin850 commented Nov 9, 2013

Would you mind trying to send a patch with a test to resolve this issue please ?

@bughit
Copy link
Contributor Author

bughit commented Nov 10, 2013

Actually this was probably never intended. What was likely intended is to mimic TemplateRenderer and allow the partial to provide content_for :content_slot to the layout, which the layout would render with yield :content_slot. That requires that the partial be rendered first.

However this too is broken as the yield args from the layout are discarded.

I am not sure what's more useful, having the partial render first and provide content to the layout or for the layout to render first and influence the rendering of the partial. Perhaps both styles should be supported via an option. Currently neither is. Whoever designed this should provide some insight.

@macmartine
Copy link
Contributor

A related issue was submitted under #12826

I just submitted a pull request with a fix: #12875

@bughit bughit added the stale label Apr 23, 2014
@rafaelfranca
Copy link
Member

This issue has been automatically marked as stale because it has not been commented on for at least
three months.

The resources of the Rails team are limited, and so we are asking for your help.

If you can still reproduce this error on the 4-1-stable, 4-0-stable branches or on master,
please reply with all of the information you have about it in order to keep the issue open.

Thank you for all your contributions.

@rails-bot
Copy link

This issue has been automatically closed because of inactivity.

If you can still reproduce this error on the 4-1-stable, 4-0-stable branches or on master,
please reply with all of the information you have about it in order to keep the issue open.

Thank you for all your contributions.

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

No branches or pull requests

5 participants