Skip to content

ReferenceHelper@insertNewBefore checks for missing coordinates before replacing values #2541

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

Merged
merged 10 commits into from
Feb 12, 2022
Merged

Conversation

orkhanahmadov
Copy link
Contributor

@orkhanahmadov orkhanahmadov commented Jan 31, 2022

This is:

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

Checklist:

Why this change is needed?

Relates to this issue. When deleting a column before the last column with the null value it incorrectly keeps deleted columns value instead of copying the null/empty value from the last column. This PR fixes this issue by checking for missing coordinates and merging them with $allCoordiantes in insertNewBefore().

@oleibman
Copy link
Collaborator

For completeness, can you change your test to assert all the values in A1-C2 after removeCol rather than just B2?.

@orkhanahmadov
Copy link
Contributor Author

orkhanahmadov commented Feb 12, 2022

For completeness, can you change your test to assert all the values in A1-C2 after removeCol rather than just B2?.

Good point. I made changes to PR, added assertions for cells in the test case. I also switched to array value checking instead of using getCell(), discovered that getCell() creates the cell if it is not available in the worksheet. If I used getCell() in a test case that solved the bug on its own, without any actual code changes. I modified my implementation to use createNewCell on the worksheet to create missing cells, but this required changing the visibility of the method from private to public

@oleibman oleibman merged commit 0eeba6d into PHPOffice:master Feb 12, 2022
@oleibman
Copy link
Collaborator

Thank you for your contribution.

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