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

fix warning when open xlsx file with thumbnail #2517

Merged
merged 2 commits into from Jan 24, 2022

Conversation

mix5003
Copy link
Contributor

@mix5003 mix5003 commented Jan 21, 2022

This is:

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

Checklist:

Why this change is needed?

this should fix #2516. by not try to open thumbnail as xml

@mix5003 mix5003 force-pushed the fix-2516 branch 2 times, most recently from 284c99a to df39b32 Compare January 21, 2022 06:12
@oleibman
Copy link
Collaborator

Your test file is very large, and I am having trouble reading it because of that, but not because of the thumbnail file. I am going to upload another, much smaller file. Do you see the same warning message when you read it? I don't see any warning messages with several different versions of PhpSpreadsheet. I do see a bit of a problem - the thumbnail fail is basically ignored rather than used when, for example, saving a copy of the original - but that's a different problem that will need a different solution.
issue.2516b.xlsx

@mix5003 mix5003 force-pushed the fix-2516 branch 2 times, most recently from 4ed28ca to 2d05f91 Compare January 24, 2022 01:50
@mix5003
Copy link
Contributor Author

mix5003 commented Jan 24, 2022

i updated test files.

I don't see any warning messages with several different versions of PhpSpreadsheet

it should show warning because this piece of code try to call loadZip every files in _rels/.rels link

$rels = $this->loadZip(self::INITIAL_FILE, Namespaces::RELATIONSHIPS);
foreach ($rels->Relationship as $relx) {
$rel = self::getAttributes($relx);
$relType = (string) $rel['Type'];
$mainNS = self::REL_TO_MAIN[$relType] ?? Namespaces::MAIN;
if ($mainNS !== '') {
$xmlWorkbook = $this->loadZip((string) $rel['Target'], $mainNS);
if ($xmlWorkbook->sheets) {
foreach ($xmlWorkbook->sheets->sheet as $eleSheet) {
// Check if sheet should be skipped
$worksheetNames[] = (string) self::getAttributes($eleSheet)['name'];
}
}
}
}

and loadZip try to open it as XML
private function loadZip(string $filename, string $ns = ''): SimpleXMLElement
{
$contents = $this->getFromZipArchive($this->zip, $filename);
$rels = simplexml_load_string(
$this->securityScanner->scan($contents),
'SimpleXMLElement',
Settings::getLibXmlLoaderOptions(),
$ns
);
return self::testSimpleXml($rels);
}

in this case docProps/thumbnail.wmf is not xml. so it should warning like my original issue
simplexml_load_string(): Entity: line 1: parser error : Start tag expected, '<' not found

but if you don't see warning message i think it up to default error_reporting level. if default error_reporting is E_ERROR (not show warning). it will not show anything. should i add error_reporting(E_WARNING); in test case? or do you have anyway to test it properly?

@oleibman
Copy link
Collaborator

Aha! Thank you for the explanation. The warning messages you see are when you execute the listWorksheetNames method, and I didn't see them because I was executing the load method. It seems to me that the listWorksheetInfo method should have the same problem as listWorksheetNames, and that your fix for one should also fix the other. Can you please add an additional test for listWorksheetInfo?

@mix5003
Copy link
Contributor Author

mix5003 commented Jan 24, 2022

added test for listWorksheetInfo

@oleibman oleibman merged commit e7b0497 into PHPOffice:master Jan 24, 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.

When open xlsx with thumbnail it has some warning.
2 participants