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.

Sign up for GitHub

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

Jump to bottom

ROB : update_page_form_field_values not failing if no fields #1346

Merged
merged 2 commits into from
Sep 18, 2022
Merged

ROB : update_page_form_field_values not failing if no fields #1346

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0eb1660
ROB : update_page_form_field_values not failing if no fields
pubpub-zz Sep 13, 2022
e48e821
add warning message
pubpub-zz Sep 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions PyPDF2/_writer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -622,6 +622,8 @@ def update_page_form_field_values(
"""
self.set_need_appearances_writer()
# Iterate through pages, update field values
if PG.ANNOTS not in page:
return
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we add the attribute if it doesn't exist, e.g.:

Suggested change
if PG.ANNOTS not in page:
return
if PG.ANNOTS not in page:
page[PG.ANNOTS] = DictionaryObject()

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

if the form has no fields to field, adding the PG.ANNOTS field may be of no use and quite confusing after (looking at the page structure you may think after there is so fields to fill up whereas there isn't)

Copy link
Member

Choose a reason for hiding this comment

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

if the form has no fields to field

I guess there was a typo? I don't understand this.

At the very least we should emit a warning ... if we are in strict mode maybe even an exception?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

if the form has no fields to be filled, adding the PG.ANNOTS field may be of no use and quite confusing after (looking at the page structure you may think after there is so fields to fill up whereas there isn't) ; more consistent with PDF philosophy : if you don't need it, it is not in the dictionary (no fields means logically no PG.ANNOTS entry)

Copy link
Member

Choose a reason for hiding this comment

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

I understand why you don't want to add it. It makes sense to me now.

However, looking at it from the users perspective: The user wants to fill a form field that doesn't exist. PyPDF2 executes sucessfully without an exception, without a warning, without a log message. No field is filled. That feels wrong to me.

A warning log message - something like "Filling fields was skipped as the page does not have any annotations" would give the user a chance to fix the code.

for j in range(len(page[PG.ANNOTS])): # type: ignore
writer_annot = page[PG.ANNOTS][j].get_object() # type: ignore
# retrieve parent field values, if present
Expand Down
6 changes: 6 additions & 0 deletions tests/test_writer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -370,11 +370,17 @@ def test_fill_form():
page = reader.pages[0]

writer.add_page(page)
writer.add_page(PdfReader(RESOURCE_ROOT / "crazyones.pdf").pages[0])

writer.update_page_form_field_values(
writer.pages[0], {"foo": "some filled in text"}, flags=1
)

# check if no fields to fill in the page
writer.update_page_form_field_values(
writer.pages[1], {"foo": "some filled in text"}, flags=1
)

writer.update_page_form_field_values(
writer.pages[0], {"foo": "some filled in text"}
)
Expand Down