Skip to content

[XLSX] Fix sheet title extract function #742

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

Closed
wants to merge 3 commits into from

Conversation

guillaume-ro-fr
Copy link
Contributor

This is:

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

Checklist:

Why this change is needed?

XLSX extractSheetTitle returned the quote character in sheet name

Fix #739

XLSX extractSheetTitle return the quote character in sheet name

Fix PHPOffice#739
@guillaume-ro-fr
Copy link
Contributor Author

There is a delta between the worksheet name and the title extractor, and this impacts several functions, including Coordinate::absoluteCoordinate.
I think it is therefore necessary to modify the test condition to exclude quotes, or to modify the storage of sheet names.

@PowerKiKi
Copy link
Member

I suppose storing in memory only non-quoted title would make sense. But I am not quite sure when quotes must be used... when writing to files only ?

@PowerKiKi
Copy link
Member

@MarkBaker do you think it would be wise to move to have only non-quoted title in memory and quote them only when writing to files ?

@PowerKiKi PowerKiKi requested a review from MarkBaker November 11, 2018 09:31
@MarkBaker
Copy link
Member

MarkBaker commented Nov 12, 2018

@MarkBaker do you think it would be wise to move to have only non-quoted title in memory and quote them only when writing to files ?

Without reading all the background to why this is a problem, I'm concerned that this is potentially going to cause problems for any formula that references a spreadsheet title that contains quotes

@PowerKiKi
Copy link
Member

I'm concerned about that too. But I think it should be rather easy to unit test quoted and non-quoted sheet name in formulas to prove that it would work. If existing tests are fixed, and new tests cover our concerns, then I'd merge it.

@PowerKiKi PowerKiKi closed this Jan 2, 2019
@PowerKiKi
Copy link
Member

This PR has been automatically closed because the develop branch was permanently deleted.

If this is still important for you, please re-submit a PR against master and mention this one in the description.

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