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

False-positive #861

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

False-positive #861

wants to merge 1 commit into from

Conversation

pedrofurtado
Copy link
Contributor

@pedrofurtado pedrofurtado commented Oct 30, 2019

@unixmonkey @mileszs With the option: raise_on_all_errors: true, even if pdf is generated successfully, is throwed an exception, because err contains the progress of process, with "Done" in the end 👍

False-positive output:

Failed to execute:
["/home/rubyuser/.rvm/gems/ruby-2.6.5@social/gems/wkhtmltopdf-binary-0.12.4/bin/wkhtmltopdf", "--encoding", "UTF-8", "--orientation", "Portrait", "--page-size", "A4", "file:////tmp/wicked_pdf20191031-9321-u7lttj.html", "/tmp/wicked_pdf_generated_file20191031-9321-1fllkce.pdf"]
Error: Error generating PDF
 Command Error: Loading pages (1/6)
[>                                                           ] 0%
[======>                                                     ] 10%
[==============================>                             ] 50%
[============================================================] 100%
Counting pages (2/6)                                               
[============================================================] Object 1 of 1
Resolving links (4/6)                                                       
[============================================================] Object 1 of 1
Loading headers and footers (5/6)                                           
Printing pages (6/6)
[>                                                           ] Preparing
[============================================================] Page 1 of 1
Done 

@pedrofurtado
Copy link
Contributor Author

@marciojg @WillRadi

@unixmonkey
Copy link
Collaborator

@pedrofurtado Interesting. It seems that you are invoking also with quiet: false, is that right?

Feel free to update the RuboCop config threshold to allow the Travis-CI builds to pass.

@pedrofurtado
Copy link
Contributor Author

pedrofurtado commented Oct 31, 2019

@unixmonkey Thanks for feedback! My code is the following:

render raise_on_all_errors: true, page_size: 'A4', orientation: 'Portrait', pdf: "my_file", encoding: 'UTF-8', template: "path/to/the/template"

The failure in TravisCI is not related to the PR code 😕 It's about the existing code in gem

@pedrofurtado
Copy link
Contributor Author

pedrofurtado commented Nov 7, 2019

@unixmonkey Was it possible to simulate the error? The false-positive 🤝

@unixmonkey
Copy link
Collaborator

wkhtmltopdf by default writes it's output to stderr, even when there is no error.

This is stated as by design here (because it can output the PDF on stdout): wkhtmltopdf/wkhtmltopdf#1980

You can change this by specifying the log_level: 'error' option, which was introduced here: #834

This should change it so that it only outputs to stderr on actual errors. I wonder if this should be the default since we always output a tempfile in this project.

Can you please check against that combination and see if it helps?

@pedrofurtado
Copy link
Contributor Author

@unixmonkey Thanks for sharing this issue in wkhtmltopdf 🤝

The workaround suggested by you works well. The following code (with log_level: error together with the rest of parameters does not have false-positive):

render log_level: 'error', raise_on_all_errors: true, page_size: 'A4', orientation: 'Portrait', pdf: "my_file", encoding: 'UTF-8', template: "path/to/the/template"

But another users that will get the gem inside projects, will get in trouble with this false-positive, if they forget the use of log_level: error. Is it valid to merge this PR or provide some workaround inside gem, without the dev need to insert log_level: error?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants