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

Handle absolute path in worksheets Target #1769

Merged
merged 7 commits into from
Apr 19, 2021

Conversation

tiagof
Copy link
Contributor

@tiagof tiagof commented Dec 29, 2020

This is:

- [X] a bugfix
- [ ] a new feature

Checklist:

Why this change is needed?

Some(*) xlsx files have the target relationship value as an absolute path which breaks the code. This aims at validating that scenario and adjusting the worksheet path calculation accordingly.

(*) To be more clear, a client is providing an xlsx file where these paths absolute. I have no idea how they generate these nor do I know how to create one myself. Nevertheless, this is a real life case.

Additionally I'd like to ask for help from the community in order to add the relevant test for his scenario since, as I said, I don't even know how to create a such a file.

Let me know any comments!

@oleibman
Copy link
Collaborator

I have created a file which uses absolute paths everywhere that did not cause a problem for Excel when it tried to open it. I will upload it to this ticket.

As for your change, I am unable to see a problem with the existing code that you are trying to change in "load". The getFromZipArchive method checks for the condition you are trying to address, so your change appears to be superfluous. It certainly is for my file - PhpSpreadsheet loads it without a problem. If you have a file where you think your change to "load" is needed, I would have to see the file in question; or, if you cannot load this file successfully, I would need to compare your code to mine.

I agree that your change to listWorksheetInfo corrects a problem. Please use the uploaded file to create a test case which can be included with your PR.
pr1769e.xlsx

@tiagof
Copy link
Contributor Author

tiagof commented Apr 19, 2021

Hi. You're right about the change in load: it is unnecessary.
I've reverted it and added the test.

@oleibman
Copy link
Collaborator

Can you revert your change to CHANGELOG.md in order to move your change forward? We can apply it afterwards.

@tiagof
Copy link
Contributor Author

tiagof commented Apr 19, 2021

Done (added it to Fixes)

@oleibman oleibman merged commit 62f5135 into PHPOffice:master Apr 19, 2021
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