Better error handling when XML file can't be parsed (shows broken XML file path). More PHP 7-ish.

[woutersamaey]

Better error handling when XML file can't be parsed (shows broken XML file path). More PHP 7-ish.

[kiatng]

The fix looses the info on the type of XML errors. Can we consider something like the following?

    /**
     * Imports XML file
     *
     * @param string $filePath
     * @return boolean
     */
    public function loadFile($filePath)
    {
        if (!is_readable($filePath)) {
            //throw new Exception('Can not read xml file '.$filePath);
            return false;
        }

        $fileData = file_get_contents($filePath);
        $fileData = $this->processFileData($fileData);
        $result = $this->loadString($fileData, $this->_elementClass);
        if (is_string($result)) {
            Mage::throwException('Cannot parse XML file at '.$filePath.PHP_EOL.$result);
        }
        return $result;
    }

    /**
     * Imports XML string
     *
     * @param  string $string
     * @return bool|string
     */
    public function loadString($string)
    {
        if (is_string($string)) {
            libxml_use_internal_errors(true);
            $xml = simplexml_load_string($string, $this->_elementClass);
            $errors = [];
            foreach (libxml_get_errors() as $error) {
                $errors[] = trim($error->message) .' at line '. $error->line;
            }
            libxml_clear_errors();
            if (!empty($errors)) {
                return implode(PHP_EOL, $errors);
            }
            if ($xml instanceof Varien_Simplexml_Element) {
                $this->_xml = $xml;
                return true;
            }
        } else {
            Mage::logException(new InvalidArgumentException('"$string" parameter for simplexml_load_string is not a string'));
        }
        return false;
    }

Sample output:

Cannot parse XML file at /var/www/.../app/code/local/.../Cof/etc/config.xml
Opening and ending tag mismatch: cof_setup line 25 and resources at line 26
Opening and ending tag mismatch: cof_setup line 21 and global at line 58
Opening and ending tag mismatch: resources line 20 and config at line 106
Premature end of data in tag global line 19 at line 107
Premature end of data in tag config line 13 at line 107

[woutersamaey]

@kiatng adding the error information changes the return type of the method and there's not much benefit. If you use a decent IDE and know which XML file is broken, your IDE will show the errors. I prefer not to change the method return type for such little benefit or we would need to do more refactoring (return a ParseResult object or something "clean" like this).

[Flyingmana]

You can add this more detailed exception as part of the existing exception by using the 3th parameter of the constructor
https://www.php.net/manual/de/exception.construct.php which then can if needed be fetched via https://www.php.net/manual/de/exception.getprevious.php

[woutersamaey]

Sure, but I would still be changing the return type of loadString() from bool to bool | string or throw an exception which would also change the way that works.

[kiatng]

LGTM.

[joshuaadickerson]

As @Flyingmana said, add the extra information as the additional parameter.

I have realized after years of making exception messages variable like that, that I shouldn't. I should add more context in the context variable.

[empiricompany]

LGTM

[woutersamaey]

@Flyingmana can we can this approved?

[luigifab]

You have forgotten spaces :)

[tmotyl]

Suggested change

	Mage::throwException('Cannot parse XML file at '.$filePath);
	Mage::throwException('Cannot parse XML file at ' . $filePath);

[tmotyl]

please explain why do we need @ here?

[fballiano]

@woutersamaey would you have time to review the comments?

[fballiano]

@woutersamaey this PR is not modifiable by admins, would you reply to the comments and/or make it modifiable?