integreat-cms icon indicating copy to clipboard operation
integreat-cms copied to clipboard

Update xhtml2pdf

Open timobrembeck opened this issue 2 years ago • 4 comments

Short description

Finally, xhtml2pdf released a new version including @charludo's upstream contribution :tada:

I did not add a new test case for the specific fixed issue, maybe this would be a good idea to make sure it isn't re-introduced in future versions.

Proposed changes

  • Update xhtml2pdf
  • Update test files

Side effects

  • Unfortunately, page number seem to have stopped working (see https://github.com/xhtml2pdf/xhtml2pdf/issues/670)
  • Apparently, it re-introduces the line reversing bug (see https://github.com/xhtml2pdf/xhtml2pdf/issues/538)

Resolved issues

Fixes: #1498 Fixes: #2022

Also, it removes the vulnerable dependency future (see https://github.com/digitalfabrik/integreat-cms/security/dependabot/20)


Pull Request Review Guidelines

timobrembeck avatar Feb 01 '23 14:02 timobrembeck

Code Climate has analyzed commit 988cd164 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (50% is the threshold).

This pull request will bring the total coverage in the repository to 75.3% (0.0% change).

View more on Code Climate.

codeclimate[bot] avatar Feb 01 '23 14:02 codeclimate[bot]

@seluianova If I understood correctly, this update also resolves #2022 / https://github.com/xhtml2pdf/xhtml2pdf/issues/662, right? Could you check whether that's true?

E.g. before: https://raw.githubusercontent.com/digitalfabrik/integreat-cms/develop/tests/pdf/files/3b02f5ea5b/Integreat%20-%20Arabisch%20-%20%D9%85%D8%B9%D9%84%D9%88%D9%85%D8%A7%D8%AA%20%D8%A7%D9%84%D9%88%D8%B5%D9%88%D9%84.pdf After: https://raw.githubusercontent.com/digitalfabrik/integreat-cms/update/xhtml2pdf/tests/pdf/files/3b02f5ea5b/Integreat%20-%20Arabisch%20-%20%D9%85%D8%B9%D9%84%D9%88%D9%85%D8%A7%D8%AA%20%D8%A7%D9%84%D9%88%D8%B5%D9%88%D9%84.pdf

But it makes https://github.com/xhtml2pdf/xhtml2pdf/issues/538 more apparent? Still struggling to figure out the relations between all those issues :sweat_smile:

timobrembeck avatar Feb 01 '23 16:02 timobrembeck

@timoludwig

If I understood correctly, this update also resolves https://github.com/digitalfabrik/integreat-cms/issues/2022 / https://github.com/xhtml2pdf/xhtml2pdf/issues/662, right? Could you check whether that's true?

Well, kind of. But #2022 was a minor cosmetic issue that didn't affect readability much.

I believe https://github.com/xhtml2pdf/xhtml2pdf/issues/538 was a bigger issue, and now it is broken again. The lines are reversed, it affects the meaning.

seluianova avatar Feb 02 '23 12:02 seluianova

@timobrembeck Hey Timo, do you think it might make sense to keep the changes in test_pdf_export.py and revert the others maybe, in order to close this PR? I guess we wouldn't want to upgrade xhtml2pdf to the current version anyway.

seluianova avatar Apr 09 '24 16:04 seluianova

@seluianova The changes in the tests only make sense when the xhtml2pdf version is upgraded (since the newer version depends on pypdf instead of PyPDF3). But yes, sadly it seems as I don't have time to contribute to xhtml2pdf in the near future :cry:

I'm going to close this PR but keep the branch, so in case we upgrade eventually, I can reuse the changes.

timobrembeck avatar Apr 22 '24 20:04 timobrembeck