search

mileszs / wicked_pdf

PDF generator (from HTML) plugin for Ruby on Rails
http://www.mileszs.com/wicked-pdf-plugin
MIT License
3.52k stars 640 forks source link

Fix: Ensure assets without extensions are handled correctly #1115

Open d4rky-pl opened 2 months ago

d4rky-pl commented 2 months ago

This PR fixes the issues reported after #1094 and related to the asset manifest implementation not looking for the assets properly.

As a side note, this will most likely be my last contribution to this project. The fix itself took me 5 minutes to implement but adding those four simple test cases took... 1.5h. I wish I was joking 🥲

If anyone still has energy it would be great to at least figure out how to upgrade Mocha to the newer version as this one has a bug that prevents stubs from clearing properly (though it would be amazing to get rid of it completely and switch to rspec)

d4rky-pl commented 1 month ago

@doits @joshuay03 would you like to test this PR in your apps and confirm this solved the issue for you? @unixmonkey @mathieujobin let me know if you need anything else to land this patch once we get a confirmation it works

joshuay03 commented 1 month ago

@doits @joshuay03 would you like to test this PR in your apps and confirm this solved the issue for you?

@d4rky-pl I can confirm that this fixes the issue we were having. Thank you for working on this, and the original PR which seems like a super useful feature!

d4rky-pl commented 1 month ago

@unixmonkey do you need anything else or can we release this as 2.8.1? :)

d4rky-pl commented 1 month ago

@mathieujobin I have no idea either but requiring extensions would be an API breaking change and I wanted to avoid that (especially considering the library is more or less on life support at this point).

My solution is based partially on how Sprockets solve that (in find_asset -> resolve -> resolve_logical_path -> resolve_under_paths) except I didn't add prioritizing which asset to pick based on the mime type because we don't know it and can't assume when using wicked_pdf_asset_base64 (as it's decoupled from the stylesheet / javascript concept).

d4rky-pl commented 1 month ago

One other thing we could do is make it fail when there's more than one asset with the same name before the extension to avoid unexpected situations (like having fonts.css and fonts.js and then wicked_pdf_asset_base64 picking one but not the other) but that's also going to be a breaking change 🤔

alessandrostein commented 1 month ago

Any plans to release 2.8.1? Thank you for all hard work here 🙌

d4rky-pl commented 1 month ago

Added more tests and a fix, good catch @chaadow! Let me know if you can think of any other cases where this could break so I can add more tests

d4rky-pl commented 1 month ago

@unixmonkey let me know if you need anything else to have this released. If there are any further bug reports related to this change, either before or after this PR is merged, feel free to @ me as well