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

Fixes #478 (/Index problem) #479

Merged
merged 20 commits into from
Nov 22, 2021
Merged

Fixes #478 (/Index problem) #479

merged 20 commits into from
Nov 22, 2021

Conversation

yasheena
Copy link
Contributor

This is the requested pull request from issue #478. Sorry, I was not yet familiar with pull requests.

After editing the document description of a PDF file, I've had some cases where getDetails() stopped reporting the /Info data to the Document class. I found the problem in the incomplete handling of the /Index area. So far it was assumed that the XRef is continuous (only one entry in the area /Index). In my cases there was more than one entry, e.g. "/Index[162 1 165 4]", which means 1 object starting at number 162 and 4 objects starting at number 165. I eliminated this problem in the file "RawDataParser.php" with this pull request.

Fixing problem of incomplete analysis of the /Index entry.
Wrong subdirectory.
Fix problem of uncomplete analysis of /Index entry.
optical changes
optical changes
optical changes
After adding a description to the file, the valid /Index entry now contains two entries (consisting of 2 values: first object number, number of objects):
/Index[2 1 21 2]
Adding test for issue 479
Code style update
Added more description and more checks.
@k00ni k00ni linked an issue Nov 12, 2021 that may be closed by this pull request
@k00ni k00ni changed the title Fix for /Index problem Fixes #478 (/Index problem) Nov 12, 2021
Issue #331 is fixed by issue #479: test updated
@yasheena
Copy link
Contributor Author

yasheena commented Nov 12, 2021

I checked the failed test: My fix also fixes the issue #331. The test was written to fail if something changes in the behavior related to issue #331. I have adapted and commented on the test accordingly.

@k00ni k00ni linked an issue Nov 12, 2021 that may be closed by this pull request
Copy link
Collaborator

@k00ni k00ni left a comment

Choose a reason for hiding this comment

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

First of all, thanks for your quick responses and good progression here! Just a few remarks.

tests/Integration/PageTest.php Outdated Show resolved Hide resolved
src/Smalot/PdfParser/RawData/RawDataParser.php Outdated Show resolved Hide resolved
yasheena and others added 5 commits November 12, 2021 16:01
optical changes
change to remove the native_function_invocation message
Co-authored-by: Konrad Abicht <hi@inspirito.de>
Added comments...
@k00ni
Copy link
Collaborator

k00ni commented Nov 15, 2021

@Connum @izabala @j0k3r and others, it would be good to get a second opinion on this. For me it looks good.

Copy link
Collaborator

@j0k3r j0k3r left a comment

Choose a reason for hiding this comment

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

Looks ok, just ran php-cs-fixer to fix the build

@Connum
Copy link
Contributor

Connum commented Nov 15, 2021

Didn't have much time to look into this, but it seems fine to me - congrats for fixing this @yasheena
Only comment I have is that "The above problem" in the comment in tests/Integration/PageTest.php doesn't relate to anything after the comment block above has been removed, so the wording should be adapted.

Changes for CS fixer
Comment update
Co-authored-by: Konrad Abicht <hi@inspirito.de>
@k00ni k00ni removed the needs work label Nov 16, 2021
Copy link
Collaborator

@k00ni k00ni left a comment

Choose a reason for hiding this comment

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

Thank you for your latest changes. It looks good now.

As always, we will keep this open for a few days so the community has a chance to comment, before we merge it.

@k00ni k00ni merged commit 768d1d6 into smalot:master Nov 22, 2021
@Reqrefusion
Copy link

Thanks. I've been waiting for this for a long time.

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.

Fix for /Index problem Does not parse pages other than the first page
5 participants