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

PDFKit 0.11 support #115

Closed
wants to merge 9 commits into from
Closed

Conversation

dhensby
Copy link
Collaborator

@dhensby dhensby commented Jan 1, 2021

Added support for v0.11 of PDFKit (which has added AcroForm support). Currently this library is incompatible with v0.11 as the relevant bootstrapping is not applied to the AcroForm.

Changes:

  • Simplified pdfkitAddPlaceholder to use the PDFDocument.initForm() and PDFDocument.formAnnotation() APIs
  • Moved the AcroForm parsing logic to its own helper getAcroForm
  • Reworked the pdfKitMock object in plainAddPlaceholder to better refelect the APIs we depend on
    1. Added pdfKitMock._root.data.AcroForm and pdfKitMock._acroform when an AcroForm is present
    2. Added pdfKitMock.initForm() and pdfKitMock.formAnnotation() methods
    3. pdfKitMock.ref() changed to be the same as PDFDocument.ref() - ie: it only accepts data arg
    4. pdfKitMock.ref() doesn't immediately modify the PDF buffer, instead this happens all at once at the end
    5. pdfKitMock.ref() now retains the data passed to it
    6. PDFKitReferenceMock now stores "additional data" inside the data prop to more accurately reflect the way data is stored in PDFKitReference
  • pdfkitAddPlaceholder no longer returns widget or form properties as these are handled internally now and users do not need to call .end() on them any more.

Todo:

  • Add a test for getAcroForm
  • Do some real world testing

@coveralls
Copy link

coveralls commented Jan 1, 2021

Pull Request Test Coverage Report for Build 353

  • 51 of 51 (100.0%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 344: 0.0%
Covered Lines: 326
Relevant Lines: 326

💛 - Coveralls

@dhensby
Copy link
Collaborator Author

dhensby commented Jan 2, 2021

I'm marking this as draft as this isn't creating the signatures quite right - from what I can see the AcroForm isn't being added to the PDF with the /Type /AcroForm attribute

Good PDF (before this work):

21 0 obj
<<
/Type /AcroForm
/SigFlags 3
/Fields [20 0 R]
>>
endobj

Bad PDF (with this PR):

11 0 obj
<<
/Fields [12 0 R]
/NeedAppearances true
/DA (/F1 0 Tf 0 g)
/DR <<
/Font <<
/F1 9 0 R
>>
>>
/SigFlags 3
>>
endobj

Other issues:

  • There is only one %%EOF in the PDFs generated this way (which is not the case previously) Don't know what I'm on about here - there's only one EOF marker in a good doc.
  • There is a lot of cruft the built in acroform support adds with little way to avoid it (without PR to pdfkit)

@dhensby dhensby marked this pull request as draft January 2, 2021 11:08
@dhensby dhensby marked this pull request as ready for review January 7, 2021 14:00
@dhensby dhensby force-pushed the pulls/pdfkit-11-support branch 2 times, most recently from e76e2fd to 73be455 Compare January 7, 2021 14:24
@dhensby
Copy link
Collaborator Author

dhensby commented Jan 7, 2021

@vbuch this is working now and green. It's not making quite the same use of the form APIs provided and has to make some modifications to the AcroForm that is generated, but it does mean that PDFs are successfully signed by the library when using pdfkit 0.11

@vbuch
Copy link
Owner

vbuch commented Jan 7, 2021

@dhensby I haven't had a good look at the code. But I really, really like it that you did it! I'll have a look ASAP.

I don't like that you needed to change plainAddPlaceholder. It's so fragile that I'm affraid changes to it will easily break it. Also, would really love to have support for both pdfkit10 and pdfkit11. Can you check if it is possible to have them both? pdfkitAddPlaceholder and pdfkit11AddPlaceholder. Don't try to reuse code but rather copy and paste into a separate helper. At some point lerna will come in this lib and then such a change will make a lot of sense. Also it won't be a breaking change but a feature one if you did it this way.

@dhensby
Copy link
Collaborator Author

dhensby commented Jan 8, 2021

I don't like that you needed to change plainAddPlaceholder.

The changes to plainAddPlaceholder don't really touch on the functionality very much. Mostly it decouples the buffer parsing that was happening in PdfSigner.sign() (which was only relevant when plainAddPlaceholder was being used) and moves it to the getAcroForm helper. Then I've updated the pdfkitReferenceMock to mock the new methods that pdfkitAddPlaceholder relies on. I think that these changes are relitively low risk as they are predominantly re-factoring, however they probably do require some real-world testing.

Also, would really love to have support for both pdfkit10 and pdfkit11

I don't see why we couldn't do that and I'll find some time to look into that.

@stale
Copy link

stale bot commented Apr 10, 2021

This issue has been automatically marked as stale because it has not had activity in the past 90 days. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Apr 10, 2021
@dhensby
Copy link
Collaborator Author

dhensby commented Apr 12, 2021

I'm planning on getting back to this soon

@stale stale bot removed the wontfix label Apr 12, 2021
@vbuch vbuch mentioned this pull request Sep 15, 2021
@stale
Copy link

stale bot commented Oct 6, 2021

This issue has been automatically marked as stale because it has not had activity in the past 90 days. It will be closed if no further activity occurs. Thank you for your contributions.

@stale
Copy link

stale bot commented Jan 19, 2022

This issue has been automatically marked as stale because it has not had activity in the past 90 days. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Jan 19, 2022
@stale stale bot closed this Jan 29, 2022
@djoli
Copy link

djoli commented Apr 21, 2022

@vbuch any possibility of looking back into this? I was dead keen on using your library to add signature support to some PDFs I am generating via pdfkit (newer version as I require Acroform support), but discovered there's currently no support beyond version 0.10.0 (after a long day of digging through source code and trial and error).

@vbuch
Copy link
Owner

vbuch commented Apr 26, 2022

I've started splitting the packages with lerna so, once I'm done, this will be back in scope. Need to find the time.

@dhensby dhensby deleted the pulls/pdfkit-11-support branch May 3, 2022 09:53
@dhensby dhensby restored the pulls/pdfkit-11-support branch May 3, 2022 09:53
@vbuch
Copy link
Owner

vbuch commented Oct 3, 2023

Hey @dhensby I'll have a look at your code here tomorrow and try to apply it on top of https://github.com/vbuch/node-signpdf/tree/feature/split

@vbuch
Copy link
Owner

vbuch commented Oct 6, 2023

@dhensby would you want to create @signpdf/placeholder-pdfkit011 based on this PR and the new structure in #194 ?

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

Successfully merging this pull request may close these issues.

4 participants