Inheritance

[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 mileszs#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

class 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.

[vlymar]

un-assigning you adam because i want to draft the actual PR text and have you look at it. feel free to look at code now though

[vlymar]

just realized my change to .prepended likely broke the alias method chain in .included

[vlymar]

so I found a way simpler way to fix their monkeypatch for both prepend and include. Also added a description for the PR, and added an explanatory comment to the test.

If the maintainers don't want to merge this in, i suppose we can try do the prepend the wrong way in our fortitude fork as well to avoid having to maintain 2 forks

[ajb]

I'm not sure I understand. What determines whether this returns true or false?

[vlymar]

If PdfHelper is included, this method gets created by alias_method_chain. If its prepended, this method does not exist

[ajb]

Ah jeez, I hadn't seen their railtie.rb. Makes sense now.

[ajb]

Let me test my Rails 5 branch with this version of wicked_pdf and our Fortitude... but this looks 👍 to me. I'm sure the wicked_pdf maintainers will be better suited to giving feedback.

[ajb]

This works as expected in our Rails 5 branch. Gonna merge this, and 👍 on you submitting upstream.