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

BIFF8 E_NOTICE undefined offset fix #1354

Merged
merged 1 commit into from
Oct 12, 2020

Conversation

aternowy
Copy link

@aternowy aternowy commented Feb 7, 2020

This is:

- [x] a bugfix
- [ ] a new feature

Checklist:

Why this change is needed?

Sheet View Settings Block should be 8-14 bytes long in BIFF8 Excel 97 according to the open office file format documentation. However access to byte 10 and 12 is not possible when record data is malformed, so getUInt2d throws notice.
image

Summary

Test access to byte offsets for "cached magnification factor" before using it. Use default values otherwise to avoid notices

@stale
Copy link

stale bot commented Apr 11, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
If this is still an issue for you, please try to help by debugging it further and sharing your results.
Thank you for your contributions.

@stale stale bot added stale and removed stale labels Apr 11, 2020
$zoomscaleInPageBreakPreview = self::getUInt2d($recordData, 10);
if ($zoomscaleInPageBreakPreview === 0) {
$zoomscaleInPageBreakPreview = 60;
if (isset($recordData[10]) && ($zoomscaleInPageBreakPreview = self::getUInt2d($recordData, 10)) === 0) {
Copy link
Member

Choose a reason for hiding this comment

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

Please avoid having assignment in condition and squash all the commit into one.

Copy link
Member

@PowerKiKi PowerKiKi left a comment

Choose a reason for hiding this comment

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

Please avoid having assignment in condition and squash all the commit into one.

Copy link
Member

@PowerKiKi PowerKiKi left a comment

Choose a reason for hiding this comment

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

Please avoid having assignment in condition and squash all the commit into one.

@stale
Copy link

stale bot commented Jun 26, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
If this is still an issue for you, please try to help by debugging it further and sharing your results.
Thank you for your contributions.

@stale stale bot added the stale label Jun 26, 2020
@MarkBaker MarkBaker removed the stale label Jun 28, 2020
@eth8505
Copy link
Contributor

eth8505 commented Jun 29, 2020

@PowerKiKi I remember we actually had the assignments outside of the the condition before but it failed the cyclomatic complexity check. To prevent having to do a larger refactoring we decided to move the assignment into the condition, epecially since this is only a small fix preventing a notice.

If you really think it's necessary we will of course try to find a solution acceptable for everyone, please let me and @aternowy know how to proceed.

@aternowy aternowy requested a review from PowerKiKi July 17, 2020 07:11
@PowerKiKi
Copy link
Member

The best would be to refactor, but if we cannot do that, we should at least avoid assignment in if condition and accept a cyclomatic complexity check fail.

@aternowy
Copy link
Author

@PowerKiKi thank you for response, we will add fix to this PR ASAP

@stale
Copy link

stale bot commented Sep 20, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
If this is still an issue for you, please try to help by debugging it further and sharing your results.
Thank you for your contributions.

@stale stale bot added the stale label Sep 20, 2020
@stale stale bot removed the stale label Sep 21, 2020
@eth8505 eth8505 force-pushed the fix/notice-biff8 branch 2 times, most recently from 6515cbd to c0f89b9 Compare September 21, 2020 16:35
@eth8505
Copy link
Contributor

eth8505 commented Sep 21, 2020

@PowerKiKi i finally got around to this. I have removed the assignments in the conditions which now causes the cyclomatic complexity problem as I said before.

Sheet View Settings Block should be 8-14 bytes long in BIFF8 Excel 97 according
to the open office file format documentation. However access to byte 10 and 12 is
not possible when record data is malformed, so getUInt2d throws notice.
@PowerKiKi PowerKiKi merged commit 96e843c into PHPOffice:master Oct 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants