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

Document Security - Coverage, Testing, and Bug-fixing #2128

Merged
merged 3 commits into from
May 29, 2021

Conversation

oleibman
Copy link
Collaborator

@oleibman oleibman commented May 29, 2021

Having a parallel project to complete cover Document Properties, I turned my attention to to Document Security. As happens, this particular change grew a bit over time.

Coverage and Testing Changes:

  • Since the Security object has no members which are themselves objects, there is no need for a deep clone. The untested __clone method is removed.
  • Almost all of the coverage for the Security Object came about through samples 11 and 41, not through formal tests with assertions. Formal tests have been added.
  • All methods now use type-hinting via the function signature rather than doc block.
  • Coverage is now 100%.

Bug:

  • Xlsx Reader was not evaluating the Lock values correctly. This revelation came as a result of the new tests ...
  • Which showed that Xlsx Reader was testing SimpleXmlElement as a boolean rather than the stringified version of that ...
  • Which didn't matter all that much because Xlsx Writer was writing the values as 'true' or 'false' rather than '1' or '0', and (bool) 'false' is true.
  • Xlsx Reader clearly needed a change. I was trying to avoid that while awaiting the namespacing change. At least this is restricted to a very small self-contained piece of the code.
  • It is less clear whether Xlsx Writer should be changed. It is true that Excel itself uses 1/0 when writing; however it is equally true that it recognizes true/false as well as 1/0 when reading. For now, I have left Xlsx Writer alone to limit the change to what is absolutely needed.

Other Changes:

  • I was at a complete loss as to what "lock revisions" was supposed to do, and it took a while to find anything on the web that explained it. Thank you, openpyxl, for coming through. I have documented it for PhpSpreadsheet now.

Miscellaneous Note:

  • There remains no support for Document Security in Xls Reader or Writer (nor in any of the other readers/writers except Xlsx).
  • No Phpstan baseline changes, possibly for the first time in any of my PRs since Phpstan was introduced.

This is:

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

Checklist:

Why this change is needed?

oleibman added 2 commits May 29, 2021 00:39
Having a parallel project to complete cover Document Properties, I turned my attention to to Document Security. As happens, this particular change grew a bit over time.

Coverage and Testing Changes:
- Since the Security object has no members which are themselves objects, there is no need for a deep clone. The untested __clone method is removed.
- Almost all of the coverage for the Security Object came about through samples 11 and 41, not through formal tests with assertions. Formal tests have been added.
- All methods now use type-hinting via the function signature rather than doc block.
- Coverage is now 100%.

<!-- end of coverage and testing changes list -->

Bug:
- Xlsx Reader was not evaluating the Lock values correctly. This revelation came as a result of the new tests ...
- Which showed that Xlsx Reader was testing SimpleXmlElement as a boolean rather than the stringified version of that ...
- Which didn't matter all that much because Xlsx Writer was writing the values as 'true' or 'false' rather than '1' or '0', and (bool) 'false' is true.
- Xlsx Reader clearly needed a change. I was trying to avoid that while awaiting the namespacing change. At least this is restricted to a very small self-contained piece of the code.
- It is less clear whether Xlsx Writer should be changed. It is true that Excel itself uses 1/0 when writing; however it is equally true that it recognizes true/false as well as 1/0 when reading. For now, I have left Xlsx Writer alone to limit the change to what is absolutely needed.

<!-- end of bug list -->

Other Changes:
- I was at a complete loss as to what "lock revisions" was supposed to do, and it took a while to find anything on the web that explained it. Thank you, openpyxl, for coming through. I have documented it for PhpSpreadsheet now.

<!-- end of other changes list -->

Miscellaneous Note:
- There remains no support for Document Security in Xls Reader or Writer (nor in any of the other readers/writers except Xlsx).
- No Phpstan baseline changes, possibly for the first time in any of my PRs since Phpstan was introduced.
@MarkBaker MarkBaker merged commit 05d3b93 into PHPOffice:master May 29, 2021
@MarkBaker
Copy link
Member

I was at a complete loss as to what "lock revisions" was supposed to do, and it took a while to find anything on the web that explained it. Thank you, openpyxl, for coming through. I have documented it for PhpSpreadsheet now.

That's a quirky twist... openpyxl was inspired by the then PHPExcel, and I spent a lot of time with the original developer Eric going through the document specs explaining aspects to him.... it's nice to see that openpyxl now helps explain things for PHPSpreadsheet

There remains no support for Document Security in Xls Reader or Writer (nor in any of the other readers/writers except Xlsx).

I was never able to figure out how security was handled for Xls; it should have been similar enough, but I could never get it working

@oleibman oleibman deleted the docsec2 branch July 1, 2021 15:50
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.

2 participants