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

Fix to render stylesheets as html safe string on Rails 6 #483

Merged
merged 5 commits into from Jan 24, 2021

Conversation

tricknotes
Copy link
Contributor

In Rails 6, the spec of ActiveSupport::SafeBuffer methods has been changed.
Previously, it behaves like just String, but now ActiveSupport::SafeBuffer exactly.
See for details: rails/rails#33990

This change breaks stylesheets rendering.
The rendered PDF has too escaped style tag.

There is a result of failing spec that I added in 760a8ed.

$ RAILS_VERSION="~> 6.0.0" bundle exec rspec spec/pdfkit_spec.rb:504
Run options: include {:locations=>{"./spec/pdfkit_spec.rb"=>[504]}}
F

Failures:

  1) PDFKit#to_pdf can deal with ActiveSupport::SafeBuffer if the HTML doesn't have a head tag
     Failure/Error: expect(pdfkit.source.to_s).to include("<style>#{File.read(css)}</style>")
       expected "&lt;style&gt;body { font-size: 20px; }&lt;/style&gt;<html><body>Hai!</body></html>" to include "<style>body { font-size: 20px; }</style>"
     # ./spec/pdfkit_spec.rb:509:in `block (3 levels) in <top (required)>'

Finished in 1.74 seconds (files took 0.38544 seconds to load)
1 example, 1 failure

Failed examples:

rspec ./spec/pdfkit_spec.rb:504 # PDFKit#to_pdf can deal with ActiveSupport::SafeBuffer if the HTML doesn't have a head tag

@@ -1,11 +1,15 @@
language: ruby

rvm:
- 2.3
- 2.4
Copy link
Contributor Author

@tricknotes tricknotes Jan 6, 2021

Choose a reason for hiding this comment

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

I dropped these Ruby versions because they don't work with Rails 6.x.
If you want to keep them, I'll update this PR.

@tricknotes
Copy link
Contributor Author

@serene How do you feel about this change?

@serene
Copy link
Contributor

serene commented Jan 18, 2021

@tricknotes

lgtm. Thanks for the clean fix and test.

I think we will drop support for ruby 2.3, 2.4. Can you add that as a note to the readme so that it's clear for users?

@tricknotes
Copy link
Contributor Author

tricknotes commented Jan 18, 2021

Thanks for your confirmation 😄

It seems there are no description about supported Ruby versions in readme for now.
If you think it very important for users, I'll add it into the top of readme.
Otherwise it is available to document somewhere, I'll update changelog.

As an alternative, adding spec.required_ruby_version = '>= 2.5' is also good to notify supported Ruby versions.
Ref: https://guides.rubygems.org/specification-reference/#required_ruby_version=

What do you prefer?

@serene
Copy link
Contributor

serene commented Jan 18, 2021

@tricknotes It's nice to have both usually, just to avoid more questions than necessary :)

Now, pdfkit supports Ruby 2.5 or later.
@tricknotes
Copy link
Contributor Author

@serene
Sorry for my late response.
I added note to readme and required_ruby_version to gemspec in 5be849c .

Could you review again ?

@serene serene merged commit a3f6c47 into pdfkit:master Jan 24, 2021
@serene serene mentioned this pull request Jan 24, 2021
@tricknotes tricknotes deleted the fix-for-rails-6 branch January 24, 2021 05:40
jasoncodes added a commit to ennova/pdfkit that referenced this pull request Jun 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants