Added SimpleXMLElement Exception check #222
Conversation
PowerKiKi
added
missing test
reader/xml
src/PhpSpreadsheet/Reader/Xml.php
Outdated
| @@ -167,6 +170,9 @@ public function listWorksheetInfo($pFilename) | |||
| 'SimpleXMLElement', | |||
| Settings::getLibXmlLoaderOptions() | |||
| ); | |||
| if (!($xml instanceof \SimpleXMLElement)) { | |||
There was a problem hiding this comment.
Should compare against false instead of checking types, so if ($xml === false).
Error message should include error details from simplexml.
src/PhpSpreadsheet/Reader/Xml.php
Outdated
| @@ -138,6 +138,9 @@ public function listWorksheetNames($pFilename) | |||
| 'SimpleXMLElement', | |||
| Settings::getLibXmlLoaderOptions() | |||
| ); | |||
| if (!($xml instanceof \SimpleXMLElement)) { | |||
| throw new Exception('SimpleXMLElement can not load ' . $pFilename); | |||
There was a problem hiding this comment.
Should compare against false instead of checking types, so if ($xml === false).
Error message should include error details from simplexml.
There was a problem hiding this comment.
function simplexml_load_string returns:
SimpleXMLElement an object of class SimpleXMLElement with * properties containing the data held within the xml document, or FALSE on failure.
It just return false when error occurred, so there's no more error information.
src/PhpSpreadsheet/Reader/Xml.php
Outdated
| @@ -344,6 +350,9 @@ public function loadIntoExisting($pFilename, Spreadsheet $spreadsheet) | |||
| 'SimpleXMLElement', | |||
| Settings::getLibXmlLoaderOptions() | |||
| ); | |||
| if (!($xml instanceof \SimpleXMLElement)) { | |||
|
@PowerKiKi I had added the unit test and change log and fixed the code style. |
|
Thanks ! |
|
@PowerKiKi my pleasure |
This is:
Checklist:
What does it change?
When the xml file get is not a standard xml file, the simplexml_load_string will return false, this will cause an error on "$xml->getNamespaces(true);" . So, instead of show the error, throw an exception may be better for it.