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

Unexpected Namespacing in rels File #3722

Merged
merged 6 commits into from
Sep 13, 2023
Merged

Conversation

oleibman
Copy link
Collaborator

@oleibman oleibman commented Sep 10, 2023

Fix #3720. Third-party product created a spreadsheet which PhpSpreadsheet could not read because of unexpected namespacing in workbook.xml.rels.

The file which demonstrated the problem was attached to #3423, however I do not believe it was related to the original problem. Nevertheless, the original issue specifically called out Protection, so I put some Protection tests in the validation test for the fix. In doing so, I found that Style/Protection is particularly confusing. Its properties will often have the value inherit, which isn't all that helpful; and, even when the locked value is protected, the cell won't actually be locked unless the sheet is protected as well. The hidden property is even more obscure - it applies only to formulas, and refers to hiding the property on the formula bar, not in the cell. I have added methods isLocked and isHiddenOnFormulaBar to Cell. I corrected the docs to explain this. And, as long as I was looking at the docs, I corrected some examples to use getHighestDataRow/Column rather than getHighestRow/Column, a frequent problem for users (e.g. #3721).

As a side note, the change to Cell.php is my first use of the nullsafe operator. This is one of many new options available now that we require Php8.0+.

Note that, even with this change, the test file is not handled perfectly. The first visible sheet has two textboxes, and PhpSpreadsheet does not handle these. Although limited support for form controls was added with PR #3130, the form controls were expected to be found in vml files. In this spreadsheet, the forms are found in a drawing xml file, but the xml tags for the textboxes are not recognized by PhpSpreadsheet. If I can figure out how to handle them, and that is far from certain, it is possible that they might be handled better than the vml files. Regardless, that work will not be part of this PR. I will open a new issue about this after this change is merged.

This is:

  • a bugfix
  • a new feature
  • refactoring
  • additional unit tests

Checklist:

  • Changes are covered by unit tests
    • Changes are covered by existing unit tests
    • New unit tests have been added
  • Code style is respected
  • Commit message explains why the change is made (see https://github.com/erlang/otp/wiki/Writing-good-commit-messages)
  • CHANGELOG.md contains a short summary of the change and a link to the pull request if applicable
  • Documentation is updated as necessary

Why this change is needed?

Provide an explanation of why this change is needed, with links to any Issues (if appropriate).
If this is a bugfix or a new feature, and there are no existing Issues, then please also create an issue that will make it easier to track progress with this PR.

Fix PHPOffice#3720. Third-party product created a spreadsheet which PhpSpreadsheet could not read because of unexpected namespacing in workbook.xml.rels.

The file which demonstrated the problem was attached to PHPOffice#3423, however I do not believe it was related to the original problem. Nevertheless, the original issue specifically called out Protection, so I put some Protection tests in the validation test for the fix. In doing so, I found that Style/Protection is particularly confusing. Its properties will often have the value `inherit`, which isn't all that helpful; and, even when the `locked` value is `protected`, the cell won't actually be locked unless the sheet is protected as well. The `hidden` property is even more obscure - it applies only to formulas, and refers to hiding the property on the formula bar, not in the cell. I have added methods `isLocked` and `isHiddenOnFormulaBar` to `Cell`. I corrected the docs to explain this. And, as long as I was looking at the docs, I corrected some examples to use `getHighestDataRow/Column` rather than `getHighestRow/Column`, a frequent problem for users (e.g. PHPOffice#3721).

As a side note, the change to Cell.php is my first use of the nullsafe operator. This is one of many new options available now that we require Php8.0+.
It's being silly again. In many tests, we test a variable for non-null, then use that variable later and Scrutinizer knows it's not null. Not here. Oh well.
Test if protected without allocating cell if it doesn't exist.
@oleibman oleibman merged commit c5527ba into PHPOffice:master Sep 13, 2023
@oleibman oleibman deleted the issue3720 branch November 13, 2023 15:41
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.

Unexpected Namespacing in workbook.xml.rels
1 participant