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

Allow Spreadsheet Serialization #4326

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

oleibman
Copy link
Collaborator

Fix #4324. Serialization was explicity forbidden by PR #3199. This was in response to several issues, and concern that the Spreadsheet object contained non-serializable properties. This PR restores the ability to serialize a spreadsheet. Json serialization remains unsupported.

Fix #1757, closed in Nov. 2023 but just reopened. At the time, Cell property formulaAttributes was stored as a SimpleXmlElement. Dynamic arrays PR #3962 defined that property as null|array<string, string> in the doc block. However, it left the formal Php type for the property as mixed. This PR changes the formal type to ?array.

Fix #1741, closed in Dec. 2020 but just reopened. Calculation property referenceHelper was defined as static, and, since static properties don't take part in serialization, this caused a problem after unserialization. There are at least 3 trivial ways to deal with this - make it an instance property, reinitialize it when unserialized using a wakeup method, or remove the property altogether. This PR uses the last of those 3.

Calculation does have other static properties. Almost all of these deal with locale. So serialize/unserialize might wind up using a default locale when non-default is desired (but not necessarily required). If that is a problem for end-users, it will be a new one, and I will work on a solution if and when the time comes.

Static property returnArrayAsType is potentially problematic. However, instance property instanceArrayReturnType is the preferred method of handling this, and using that will avoid any problems.

Issue #932 also dealt with serialization. I do not have the wherewithal to investigate that issue. If it is not solved by this and the earlier PR's, I will have to leave it to others to re-raise it.

Spreadsheet copy is now simplified to use serialize followed by unserialize. Formal tests are added. In addition, I have made a number of informal tests on very complicated spreadsheets, and it has performed correctly for all of them.

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#4324. Serialization was explicity forbidden by PR PHPOffice#3199. This was in response to several issues, and concern that the Spreadsheet object contained non-serializable properties. This PR restores the ability to serialize a spreadsheet. Json serialization remains unsupported.

Fix PHPOffice#1757, closed in Nov. 2023 but just reopened. At the time, Cell property `formulaAttributes` was stored as a SimpleXmlElement. Dynamic arrays PR PHPOffice#3962 defined that property as `null|array<string, string>` in the doc block. However, it left the formal Php type for the property as `mixed`. This PR changes the formal type to `?array`.

Fix PHPOffice#1741, closed in Dec. 2020 but just reopened. Calculation property `referenceHelper` was defined as static, and, since static properties don't take part in serialization, this caused a problem after unserialization. There are at least 3 trivial ways to deal with this - make it an instance property, reinitialize it when unserialized using a wakeup method, or remove the property altogether. This PR uses the last of those 3.

Calculation does have other static properties. Almost all of these deal with locale. So serialize/unserialize might wind up using a default locale when non-default is desired (but not necessarily required). If that is a problem for end-users, it will be a new one, and I will work on a solution if and when the time comes.

Static property `returnArrayAsType` is potentially problematic. However, instance property `instanceArrayReturnType` is the preferred method of handling this, and using that will avoid any problems.

Issue PHPOffice#932 also dealt with serialization. I do not have the wherewithal to investigate that issue. If it is not solved by this and the earlier PR's, I will have to leave it to others to re-raise it.

Spreadsheet `copy` is now simplified to use serialize followed by unserialize. Formal tests are added. In addition, I have made a number of informal tests on very complicated spreadsheets, and it has performed correctly for all of them.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
1 participant