Fix Reading XLSX files without styles.xml throws an exception. #2247
Conversation
Suggestions for PR PHPOffice#2247.
There was a problem hiding this comment.
Thank you for identifying this problem. I believe that your proposed solution is more complex than warranted. This isn't entirely your problem; the code into which your solution must go is already very complicated. Nevertheless, if you look at PR #2248, I believe you will find that it addresses your problem in a manner which is simpler and clearer to review. It would have been even simpler, but you've exposed a second problem which I believe your change does not address - if the Styles file exists, but doesn't actually contain any styles, read will fail for an entirely different reason. I am uploading a file which meets those criteria.
The PR which I mentioned in the previous paragraph is in draft status. I have no intention of poaching on your idea and moving it forward. If you would use it as the basis for updating you PR, I can see about proceeding with yours.
issue.2246b.xlsx
|
I can see what you mean. The fix is not really as complex as it seems on the review, is just a check on the xpath, but I understand it may become problematic as there are too many lines being ignored when $xpath is null and doesn't address the problem of empty styles.xml files I will try to take #2248 as basis and see if can make this work. Thank you. |
|
It seems that my solution was entirely not necessary, I didn't know about testSimpleXml and it fixes all the issues when no file was present. I also tested this solution with empty styles.xml and it seems that no error was thrown in this case, I added a test for it anyway. |
|
My apologies - I didn't explain the "empty styles" problem well enough. As your tests show, you can read the file with empty styles without a problem. But, if you then try to save the spreadsheet, you will encounter errors on write. That was why I moved the removeCellStyleXfByIndex and removeCellXfByIndex statements in my PR; it eliminates that problem. Your PR should make those changes as well. It also requires slightly more extensive tests. You want to write and reload the spreadsheet after reading it in in each of your new tests, in order to make sure that it's usable. An example of an existing test which does this is |
|
I saw the other changes but I thought they were unrelated and that's why I didn't add them, sorry for that. Those are added now too. Also changed the test to write and reload the files and check the first cell value to ensure that everything can be read again. Didn't extend the AbstractFunctional from XlsxTest as didn't really like the idea of adding that dependency into XlsxTest class so I wrote the reload code directly on the tests but you may think otherwise. Sorry for all this, I really had no problem with you "poaching my idea", I wouldn't mind if you just merged your changes, just didn't understand what you meant at first :) |
|
I tried downloading the spreadsheets against which you are running the new tests, and Excel is unable to open the files. I really am not able to figure out why. I can open both of them with OpenOffice. Can you substitute your original file and the one I uploaded? |
|
I replaced them again with the "original" ones. I though having a simplified file for testing would be better, but I don't have Microsoft Office for testing, I guess we are ok going on with these files. |
|
Thanks. Those files open just fine in Excel. |
|
At the moment, I do not see how to merge your change. Beside the message "This branch is out-of-date with the base branch", there is usually a button "Update branch". I have occasionally seen that button grayed out because of merge conflicts. But, until now, I have never seen it missing altogether. I am not a Github expert. Do you have any idea how to resolve this? |
This is an exact replacement for PR PHPOffice#2247 by @arraintxo. That PR has reached a state in Github where it cannot be merged, and I am not sure how to fix it, other than by re-submitting it as this PR. See issue PHPOffice#2246. A spreadsheet lacking styles.xml is throwing an exception on Read. Additionally, after fixing that problem, attempting to save the object that has been read throws an exception. This is also true for a spreadsheet with an empty styles.xml.
|
I have submitted PR #2272 as a draft. If you want to try to fix whatever is wrong with this one, I will wait; otherwise, I'll move forward with the new one. Please let me know how you you wish to proceed. |
|
Sorry, I was out all the weekend and didn't read this until know. I just clicked on the "Update branch" button. I really don't mind how you proceed, any procedure that makes you're life easier is good for me. |
|
Glad you were able to update it. Thank you for your contribution. |
…fice#2247) * Fix Reading XLSX files without styles.xml throws an exception. * Bugfix, debugging code removed * Fix Reading XLSX files without styles.xml throws an exception (rethinked) * Fix Reading XLSX files without styles.xml throws an exception (rethinked) * Style fixes * Fix Spreadsheet loaded without styles cannot be written * Replaced test files for empty styles.xml testing
This is:
Checklist:
Why this change is needed?
Reading XLSX files without styles.xml is throwing an exception and this should not happen.