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

Possible fix for issue #3337 #3338

Merged
merged 4 commits into from
Feb 9, 2023
Merged

Possible fix for issue #3337 #3338

merged 4 commits into from
Feb 9, 2023

Conversation

fdjohnston
Copy link
Contributor

Using the FunctionPrefix::addFunctionPrefixStripEquals appears to fix the issue by adding the _xlfn prefix to functions in array formulas.

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?

Addresses issue #3337.

I didn't write any unit tests since I wasn't sure how to test the underlying XML output by the writer. Happy to take a crack at it if you have any tips.

Using the `FunctionPrefix::addFunctionPrefixStripEquals` appears to fix the issue by adding the _xlfn prefix to functions in array formulas.
@oleibman
Copy link
Collaborator

oleibman commented Feb 2, 2023

Take a look at the test member Writer\Xlsx\ConditionalTest. Those tests make assertions about the generated Xml (without even having to write to an output file).

Added two unit tests for issue #3337: one that includes the xlfn prefix, and one that does not.  These tests will ensure that the prefix is being added, when appropriate, within an array formula.
@fdjohnston
Copy link
Contributor Author

Thanks for the recommendation. I started with a couple simple cases. These can be expanded upon if necessary, though based on @MarkBaker comments back in issue #3337 it sounds like this is all getting overhauled in the next major version of PhpSpreadsheet anyway.

@MarkBaker
Copy link
Member

based on @MarkBaker comments back in issue #3337 it sounds like this is all getting overhauled in the next major version of PhpSpreadsheet anyway.

That doesn't change the need to write the function prefix though

@fdjohnston
Copy link
Contributor Author

Totally agree. Was just suggestion that a couple of simpler tests is probably adequate.

@fdjohnston
Copy link
Contributor Author

But of course that's up to you. Happy to add others if you feel it's necessary.

@MarkBaker MarkBaker merged commit 38ea66d into PHPOffice:master Feb 9, 2023
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.

3 participants