Getting more information about failing xPath selector #361

OzzyCzech · 2017-01-17T12:50:41Z

BC break:

Newly throws InvalidArgumentException during processing invalid
xPath selectors instead of silent PHP warning.

It’s related to #360 issue

OzzyCzech · 2017-01-17T12:55:50Z

Looks that HHVM do not fire Exception at all :(

OzzyCzech · 2017-01-17T12:57:37Z

facebook/hhvm#5790

oliverklee · 2017-01-17T12:54:08Z

Classes/Emogrifier.php

+     *
+     * @throws \InvalidArgumentException
+     */
+    public function errorHandler($errNo, $errStr, $errFile, $errLine, $errContext)


Please let's make this more specific, e.g., handleXpathError.

oliverklee · 2017-01-17T12:54:53Z

Classes/Emogrifier.php

@@ -1501,4 +1505,34 @@ private function getNodesToExclude(\DOMXPath $xPath)

        return $excludedNodes;
    }
+
+    /**
+     * Handle warnings in process() method


The first line of a method comment should always be in the third person: "[This method] Handles …"

oliverklee · 2017-01-17T12:55:13Z

Classes/Emogrifier.php

+     * @param string $errStr
+     * @param string $errFile
+     * @param int $errLine
+     * @param array $errContext


Do we know more about the array, e.g., string[][] or int[][]?

context it's just an array of object contains context variables from current scope

Do we know anything about the types of the contained context variables? Or will they be mixed int, string, bool etc?

No, they will be arrays, objects and strings ... mixed

Okay, then "array" will suffice. Thanks for the info!

oliverklee · 2017-01-17T12:55:40Z

Classes/Emogrifier.php

+     *
+     * @throws \InvalidArgumentException
+     */
+    public function errorHandler($errNo, $errStr, $errFile, $errLine, $errContext)


Please add an "array" type hint to the last parameter.

oliverklee · 2017-01-17T12:56:35Z

Classes/Emogrifier.php

+     * @param int $errLine
+     * @param array $errContext
+     *
+     * @return bool


We don't need a return value as this method throws an exception if there is a problem and does not throw it if everything is fine. So this can be return void, and the return false can go away.

Return false have specific reason, we don't want to catch all errors and throw them away
If the function returns FALSE then the normal error handler continues.

Ah, okay, I didn't know that. Thanks for the explanation. Could you then please add something like "bool always false" plus an explanation to the return annotation? Thanks!

oliverklee · 2017-01-17T12:57:30Z

Classes/Emogrifier.php

+     *
+     * @throws \InvalidArgumentException
+     */
+    public function errorHandler($errNo, $errStr, $errFile, $errLine, $errContext)


No abrreviations ("err"), please. Actually, I think that we don't need the prefix anyway as the context already is given by the method.

oliverklee · 2017-01-17T12:58:02Z

Classes/Emogrifier.php

@@ -1501,4 +1505,34 @@ private function getNodesToExclude(\DOMXPath $xPath)

        return $excludedNodes;
    }
+
+    /**
+     * Handle warnings in process() method


Please also add some description on what the method actually does.

oliverklee · 2017-01-17T12:58:32Z

Classes/Emogrifier.php

+     */
+    public function errorHandler($errNo, $errStr, $errFile, $errLine, $errContext)
+    {
+        if ($errNo === E_WARNING && isset($errContext['cssRule']['selector'])) {


Do we need this check, or will the handler also be called in other cases?

not really :) but

code sniffer trigger error about not used variable

errorHandler will be called even for E_ERRORS

oliverklee · 2017-01-17T13:00:10Z

On the HHVM problem: Can we work around this somehow? Or document that this feature only is available on PHP, not on HHVM, and skip the test in that case?

oliverklee · 2017-01-17T13:01:14Z

Could you also please add a line to the change log file (newest on top) and add a "Fixes #360" to the body of the commit message so the other issue will be auto-closed on merging? Thanks!

OzzyCzech · 2017-01-17T13:53:14Z

Only way is skip this test in HHVM

oliverklee · 2017-01-17T15:43:51Z

Okay, then we need to skip it depending on the runtime. Here is an example: sebastianbergmann/phpunit#1356

OzzyCzech · 2017-01-18T08:40:26Z

done, check latest version

oliverklee · 2017-01-18T12:41:24Z

@OzzyCzech Thanks! I'll have a look at it!

oliverklee

This looks very good, but still needs some minor changes.

oliverklee · 2017-01-19T17:19:32Z

Classes/Emogrifier.php

        $this->process($xmlDocument);
+        restore_error_handler();


We have exactly the same code here and in emogrify (starting from the check for empty $this->html). Please let's move this to a separate private method to avoid the duplication.

Maybe we can move handler code to process() or add processWithErrorHandling() method

oliverklee · 2017-01-19T17:20:00Z

Classes/Emogrifier.php

+
+    /**
+     * Handles invalid xPath expression warnings, generated by process() method,
+     * during querying \DOMDocumentand and trigger \InvalidArgumentException


There's a space missing before the "and".

oliverklee · 2017-01-19T17:20:14Z

Classes/Emogrifier.php

+    /**
+     * Handles invalid xPath expression warnings, generated by process() method,
+     * during querying \DOMDocumentand and trigger \InvalidArgumentException
+     * with invalid selector


Please add the missing period at the end.

oliverklee · 2017-01-19T17:21:03Z

Tests/Unit/EmogrifierTest.php

+        // HHVM ignore custom error handler settings for libxml
+        // @see https://github.com/facebook/hhvm/issues/5790
+        if (defined('HHVM_VERSION')) {
+            $this->markTestIncomplete('HHVM ignore custom error handler');


This should be markTestSkipped, not markTestIncomplete. (markTestIncomplete serves as a kind of "to do" marker.)

oliverklee · 2017-01-20T10:52:22Z

Classes/Emogrifier.php

+     *
+     * @throws \InvalidArgumentException
+     */
+    public function handleXpathError($type, $message, $file, $line, array $context)


Does this method need to be public, or could it be private?

It's have to be public, it's callback. But there is one option. Callback can be hidden to directly to the process like set_error_handler(function() { }, ...); but that's avoid options to overwrite handler behavior.

For me is clear api better then have this function in public space

I agree. Then let's keep it this way. Thanks for the explanation!

Shall I leave it as is or hide callback to process?

Please leave it as a public method. Thanks!

:) ok, check latest version

oliverklee · 2017-01-20T10:53:50Z

Classes/Emogrifier.php

+     *
+     * @return void
+     */
+    private function processWithErrorHandling(\DOMDocument $xmlDocument)


Thanks for moving this to a separate method.

process now only is called from this method. So I think the error handling could go directly into process. What do you think?

Sure, that can be part of process method

oliverklee · 2017-01-20T10:54:11Z

Classes/Emogrifier.php

+    /**
+     * Handles invalid xPath expression warnings, generated by process() method,
+     * during querying \DOMDocument and trigger \InvalidArgumentException
+     * with invalid selector


Please add the missing period at the end of the sentence.

oliverklee · 2017-01-20T11:19:15Z

CHANGELOG.md

@@ -25,7 +25,12 @@ This project adheres to [Semantic Versioning](http://semver.org/).

 ### Security

+## 1.2.0


This line should not be here. (We'll add it later for the release.)

oliverklee · 2017-01-20T11:19:40Z

CHANGELOG.md


+### Added


There's also already an "Added" section (line 9) where you can add the feature.

I am blind 🤦‍♂️

oliverklee · 2017-01-20T11:20:50Z

Classes/Emogrifier.php

@@ -329,6 +329,7 @@ public function emogrifyBodyContent()
     */
    protected function process(\DOMDocument $xmlDocument)


This method now needs the "throws" annotation (and an explanation on the same line explaining that the Exception is thrown for invalid XPath).

oliverklee · 2017-01-20T11:21:59Z

Classes/Emogrifier.php

@@ -402,6 +403,7 @@ protected function process(\DOMDocument $xmlDocument)
        }

        $this->copyCssWithMediaToStyleNode($xmlDocument, $xPath, $cssParts['media']);
+        restore_error_handler();


Do the two lines need enclose this part of the code, or can we narrow it down so it encloses only the lines that actually do some XPath processing?

I am just move both lines closer to the queries, that's better

BC break: > Newly throws InvalidArgumentException during processing invalid > xPath selectors instead of silent PHP warning. Fixes #360

oliverklee

Looks great. Thanks!

OzzyCzech · 2017-01-20T12:00:04Z

You are welcome!

oliverklee assigned OzzyCzech Jan 17, 2017

oliverklee added this to the 1.2.0 milestone Jan 17, 2017

oliverklee added the enhancement label Jan 17, 2017

oliverklee requested changes Jan 17, 2017

View reviewed changes

oliverklee requested changes Jan 19, 2017

View reviewed changes

oliverklee requested changes Jan 20, 2017

View reviewed changes

Getting more information about failing xPath selector

ec24b3c

BC break: > Newly throws InvalidArgumentException during processing invalid > xPath selectors instead of silent PHP warning. Fixes #360

oliverklee added the in review label Jan 20, 2017

oliverklee added 2 commits January 20, 2017 12:58

Update CHANGELOG.md

3b222a1

Update CHANGELOG.md

1064bc4

oliverklee approved these changes Jan 20, 2017

View reviewed changes

oliverklee merged commit 58df2e7 into MyIntervals:master Jan 20, 2017

oliverklee removed the in review label Jan 20, 2017

unfulvio mentioned this pull request Mar 28, 2017

[3.0] Update Emogrifier class for Php7.1 support and bugfixes woocommerce/woocommerce#13788

Closed

jriboux mentioned this pull request Jun 22, 2017

Fix InvalidArgumentException #392

Merged

markhalliwell mentioned this pull request Sep 17, 2017

[BUGFIX] Silence purposefully ignored PHP warnings #400

Merged

JakeQZ mentioned this pull request Sep 11, 2019

Errors with malformed CSS #179

Closed

		@@ -25,7 +25,12 @@ This project adheres to [Semantic Versioning](http://semver.org/).

		### Security

		## 1.2.0

		@@ -329,6 +329,7 @@ public function emogrifyBodyContent()
		*/
		protected function process(\DOMDocument $xmlDocument)

Getting more information about failing xPath selector #361

Getting more information about failing xPath selector #361

Conversation

OzzyCzech commented Jan 17, 2017

OzzyCzech commented Jan 17, 2017

OzzyCzech commented Jan 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliverklee commented Jan 17, 2017

oliverklee commented Jan 17, 2017

OzzyCzech commented Jan 17, 2017

oliverklee commented Jan 17, 2017

OzzyCzech commented Jan 18, 2017

oliverklee commented Jan 18, 2017

oliverklee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliverklee left a comment

Choose a reason for hiding this comment

OzzyCzech commented Jan 20, 2017