Handle PdfHelper include vs prepend with separate modules

[adzap]

As a solution to the current infinite loop issue, I've approached it by handling include vs prepend slightly differently so that you are just using alias_method_chain or super respectively. No getting these jumbled by using a single module to handle both cases.

[adzap]

Simplified a few times. Offering this PR as a way to fix #810.

Build will pass, just needs a kick again.

[unixmonkey]

@adzap I've restarted those, and they have indeed passed, but I pulled this change into an older project I've managed to reproduce the issue in, and it still gets into an infinite loop on render.

For reference:
Ruby 2.2.2
Rails 4.2.1
Turbolinks 2.3.0 (may not be relevant)
Devise 3.5.1
remotipart 1.4.2
OSX Mojave

[adzap]

@unixmonkey thanks for testing. I don't use remotipart but had an infinite loop issue, which this PR solves. Seems the remotipart gem causes a variation this PR doesn't fix. I look into remotipart.

[unixmonkey]

@adzap Yep. Just adding it to your Gemfile should be enough to get an insta-crash, I think.

[adzap]

@unixmonkey I can see remotipart is trying to avoid different override techniques and just stick with aliasing. The causes a conflict if wicked_pdf uses prepend and remotipart is aliasing our render override as without we get a loop.

Would be great if remotipart supported prepend, but until then we need to work around it. Options I see:

1. Force wicked_pdf to load after remotipart by adding :after "remotipart.controller_helper" to the initializer block. This could be config as list of other initializer blocks to load after.
2. Get remotipart to use prepend when available.
3. Add a README note to tell people to load wicked_pdf last if an infinite loop occurs by adding require: false to gem 'wicked_pdf' and adding config.after_initialize { require 'wicked_pdf' } to force it load last.

[unixmonkey]

@adzap That's great! I didn't realize you could order initializers like that.
This seems to solve the immediate problem, and doesn't seem to have any negative effects when that gem/initializer isn't in the project. I'm inclined to release a version with just that addition like so:

initializer 'wicked_pdf.register', :after => 'remotipart.controller_helper' do |_app|
  # include/prepend stuff
end

Do you see any issues with that approach?

[adzap]

It would good to add it as config and make it empty. Then instruct people to add it the initialiser names as required eg the remotipart one. That way we support any issue like this without a problem. Don’t like idea of adding code specific to some other random unrelated gem.

…

On 26 Mar 2019, 11:10 AM +1100, David Jones ***@***.***>, wrote: @adzap That's great! I didn't realize you could order initializers like that. This seems to solve the immediate problem, and doesn't seem to have any negative effects when that gem/initializer isn't in the project. I'm inclined to release a version with just that addition like so: initializer 'wicked_pdf.register', :after => 'remotipart.controller_helper' do |_app| # include/prepend stuff end Do you see any issues with that approach? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[unixmonkey]

I don’t see a good way to do that since the railtie is loaded before the wicked_pdf.rb initializer.

[adzap]

You're right, I hadn't thought of that. Then option 3 seems like the way forward. I don't like this idea of putting in work arounds for some other badly behaving gem.

[adzap]

Can you tell me if just moving the remotipart gem declaration in your Gemfile above wicked_pdf fixes this?

…

On 26 Mar 2019, 12:27 PM +1100, David Jones ***@***.***>, wrote: I don’t see a good way to do that since the railtie is loaded before the wicked_pdf.rb initializer. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[unixmonkey]

No, changing the position in the Gemfile appears to have no effect. I put remotipart at the top, and wicked_pdf at the bottom with no change in behavior.

Option 3 also doesn't work for me in this app, but it seems to be because config/initializers/wicked_pdf.rb is evaluated before config.after_initialize, and since it isn't required, the constant doesn't exist yet.

Regarding passing configuration to the railtie, we can do that if it's defined in config/application.rb like this:

module MyApp
  class Application < Rails::Application
    config.wicked_pdf.skip_render_overrides = true
    config.wicked_pdf.skip_asset_helpers = true

if the railtie is something like this:

class WickedRailtie < Rails::Railtie
  config.wicked_pdf = ActiveSupport::OrderedOptions.new

  initializer 'wicked_pdf.register' do |app|
    unless app.config.wicked_pdf.skip_render_overrides
      # includes/prepends
    end
    unless app.config.wicked_pdf.skip_asset_helpers
      # asset include
    end

Then manually include WickedPdf::PdfHelper in ApplicationController (or a specific controller).

Allowing you to opt-in or out of magic stuff in the railtie is on my radar anyway, but I'm not sold on the naming, since you could skip_render_overrides = true, but if you include it in your controller, you are still overriding, just later in the process.

[adzap]

I think I would probably invert the naming logic to something like enable_render_overrides = true and enable_asset_helpers = true.

Also rather than creating parallel gem config, I would do this

config.wicked_pdf = ::WickedPdf.config in the Railtie and add these new config options to the gem config object.

I think the future should be that we don't need a PdfHelper at all. With my suggested extraction of the controller render helper methods into a Renderer object and defining a ActionController::Renderers.add :pdf block this works very cleanly without any overriding or helper module. We should remove the render_to_string method completely and use a Renderer a object directly.

[adzap]

To add this config and extra complexity now seems more problematic than just moving to the Renderer solution which solves the render method problem as Rails intended and we can leave the render_to_string override in place because it is far less contentious a method. I can fix up the existing PR and remove some of the unnecessary commits. Let me know if you’re up for it.

…

On 27 Mar 2019, 3:18 AM +1100, David Jones ***@***.***>, wrote: No, changing the position in the Gemfile appears to have no effect. I put remotipart at the top, and wicked_pdf at the bottom with no change in behavior. Option 3 also doesn't work for me in this app, but it seems to be because config/initializers/wicked_pdf.rb is evaluated before config.after_initialize, and since it isn't required, the constant doesn't exist yet. Regarding passing configuration to the railtie, we can do that if it's defined in config/application.rb like this: module MyApp class Application < Rails::Application config.wicked_pdf.skip_render_overrides = true config.wicked_pdf.skip_asset_helpers = true if the railtie is something like this: class WickedRailtie < Rails::Railtie config.wicked_pdf = ActiveSupport::OrderedOptions.new initializer 'wicked_pdf.register' do |app| unless app.config.wicked_pdf.skip_render_overrides # includes/prepends end unless app.config.wicked_pdf.skip_asset_helpers # asset include end Then manually include WickedPdf::PdfHelper in ApplicationController (or a specific controller). Allowing you to opt-in or out of magic stuff in the railtie is on my radar anyway, but I'm not sold on the naming, since you could skip_render_overrides = true, but if you include it in your controller, you are still overriding, just later in the process. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[unixmonkey]

I agree with the sentiment, but I think it's important to go through a deprecation cycle and throw warnings for stuff we are going to remove. This is a very widely used library, and it's already very tricky to upgrade wkhtmltopdf.

I'd rather throw a temporary hack in to make this a safe upgrade for 1.x users, with a path to use a new renderer in preparation for a 2.x, because I guarantee people are out there using parts of the API we would be removing.

[adzap]

Ok I understand. The render method is the public API so a change to that would be a semver-like break. You have the experience with the user community so I defer to you on that.

Unfortunately these config changes won't be a fix for me as my wicked_pdf usage is from another gem which I'm not inclined to change it to add this fix as it requires config change and changes to controller code. So I'll probably have to release my own fork gem.

Unless you have a suggested path to a v2 alpha/beta release I can assist with in the short term?

[23tux]

I have the same issue when using it together with view_component (see ViewComponent/view_component#332). Any updates on this?