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

359: Add block back into render call #360

Merged
merged 1 commit into from
Mar 6, 2018

Conversation

dorner
Copy link
Contributor

@dorner dorner commented Feb 28, 2018

No description provided.

@delner
Copy link
Contributor

delner commented Feb 28, 2018

Interesting, and also a bit surprising this wasn't being passed. Do you have an example of something passing a block to render?

@delner delner self-requested a review February 28, 2018 21:20
@delner delner added this to the 0.11.3 milestone Mar 1, 2018
Copy link
Contributor

@delner delner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like we also aren't passing blocks for other render functions.

Could you add the same to https://github.com/wishabi/dd-trace-rb/blob/b3f848044035fd330fc43fcee80200c633c596c4/lib/ddtrace/contrib/rails/core_extensions.rb#L98?

@delner delner added bug Involves a bug community Was opened by a community member labels Mar 1, 2018
@dorner
Copy link
Contributor Author

dorner commented Mar 2, 2018

@delner render_partial doesn't take a block like render does. The block is handled separately. Same for _render_template.

For an example, here's the way it looks in our code:

<%=
  render :layout => 'ad_set_group_tabs',
    :locals => {
    :ad_sets_in_groups => @ad_sets_in_groups,
    :remote => true,
    :show_workflow => true
  } do |ad_sets, id|
    show_ad_set_group_gma_campaign_path(:group_id => id || -1)
  end
%>

This is definitely a rather obscure way of using render and I'm not surprised it hasn't been brought up before. :) Here's another example: https://stackoverflow.com/questions/2951105/rails-render-partial-with-block

@delner
Copy link
Contributor

delner commented Mar 2, 2018

Cool, the context there is helpful.

But I think the point of adding block to the partial renderer still applies, since the PartialRenderer does actually take a block for render and the tracer isn't fowarding the block. Or am I misunderstanding?

@dorner
Copy link
Contributor Author

dorner commented Mar 2, 2018

@delner yep. :) If you follow the code, here's what's happening:

  • PartialRender calls render
  • This takes a block, i.e. a Proc as an argument (not an actual Ruby block)
  • The block is saved in an instance variable
  • render then calls render_partial without any arguments
  • render_partial uses the instance variable saved previously to render

The patcher patches the render_partial method, not the render method, so all of the block handling should be managed entirely without the patched method caring about it.

Just took a closer look - so the patcher does patch the render method, but since the block being passed is an actual variable and not a Ruby block, it should be covered by *args.

@delner
Copy link
Contributor

delner commented Mar 6, 2018

Oh, whoops, you're right: block is being passed as a normal parameter in the PartialRenderer. My eye was just automatically adding the & to it. Looks like the &block in def render_with_datadog(*args, &block) patch for PartialRenderer doesn't actually match the parameter list for the function its displacing.

We should probably remove that extraneous &block from that function def, but otherwise it looks like it shouldn't suffer the same issue. Thanks for correcting me!

@delner delner merged commit d9bed2a into DataDog:master Mar 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Involves a bug community Was opened by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants