Use inheritance when prepending - Fixes conflict with other render gems

[vlymar]

After upgrading to rails 5 we found that the way wicked_pdf prepends ActionController::Base#render causes an infinite loop when combined with another gem prepending render.

I believe our issue is generally related to #111, though this PR focuses on prepend behavior.

The loop happens because wicked_pdf is not using the traditional prepend inheritance pattern, which looks like this

class Base
  def render
  end
end

module Patch
  def render
    patched_behavior
    super
  end
end

Base.prepend(Patch)

Instead, PdfHelper aliases the current render method away, then defines its own render method on the base class, which then calls the new render_with_wicked_pdf method.

If any other gem prepends its own render method using the inheritance pattern, before wicked_pdf prepends PdfHelper, this happens:

1. ActionController::Base is loaded, and its render method is defined.
2. OtherGem has its patch prepended, putting its render method in front of ActionController::Bases render method in the lookup chain. (OtherGem calls super to get to the original render method)
3. wicked_pdf has PdfHelper prepended
4. PdfHelper.prepended aliases OtherGem#render to render_without_wicked.
5. PdfHelper.prepended re-defines render on ActionController::Base using base_eval.
6. render is called from a controller action:
7. OtherGem#render is called, and it calls super
8. PdfHelper's version of #render is called, and it calls render_with_wicked
9. render_with_wicked calls render_without_wicked
10. render_without_wicked is an alias for OtherGem#render
11. infinite loop.

I've included a test that demonstrates this behavior. Its quite ugly as it has to replicate what railtie.rb does after another gem is prepended, which i couldn't figure out how to do since all of wicked is loaded before the tests run, so my solution was to just demonstrate it with a mock ActionController::Base class.

I'm new to test-unit so please let me know if these tests can be cleaned up.

[unixmonkey]

Very cool. This has been an open issue for awhile with remotipart, turbolinks, and possibly others

My desired approach in the future would be to eliminate hooking into render at all (and instead calling wicked_pdf(options) in your controller, but this could be great for those that depend on this behavior or are upgrading.

I'll take a closer look soon. Do you think this should increment the MAJOR or MINOR value if conforming to Semver?

Can you forsee be any backwards compatibility concerns with this? I do see you are checking respond_to?(:render_without_wicked_pdf), which looks like it would cover it, and it's only called on platforms that use prepend.

Thanks a ton for giving this a proper look!

[vlymar]

No problem! Thanks for the prompt response :)

I think this is a PATCH level change...as far as I can tell I haven't changed any functionality.

The only thing I can think of that could break backwards compatibility is super_method, which only exists on ruby >= 2.2. Like you said, currently it would only ever be called with rails 5, but if down the line someone accidentally removes the alias_method_chain for rails < 5, its possible render_with_wicked_pdf will fall through to the else and call super_method on a ruby that doesn't have it... I think thats fairly far fetched though, and perhaps can be caught with a test.

Personally I like the Renderers.add approach the most (that feature appears to exist as early as rails 3). It would let you call render pdf: 'whatever' without any monkeypatching at all.
#111 describes a patch that uses add_renderer. I guess the problem there is Renderers.add might not handle render_to_string, though i'm not sure what the use case for that is. At the very least you reduce the monkeypatching by 50%

[unixmonkey]

I've been sitting on this excellent work for too long. Thanks a ton for the help.

[parvesh-hyperiq]

Fixes are added to master branch.