-
Notifications
You must be signed in to change notification settings - Fork 154
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
Getting more information about failing xPath selector #361
Conversation
Looks that HHVM do not fire Exception at all :( |
* | ||
* @throws \InvalidArgumentException | ||
*/ | ||
public function errorHandler($errNo, $errStr, $errFile, $errLine, $errContext) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please let's make this more specific, e.g., handleXpathError.
@@ -1501,4 +1505,34 @@ private function getNodesToExclude(\DOMXPath $xPath) | |||
|
|||
return $excludedNodes; | |||
} | |||
|
|||
/** | |||
* Handle warnings in process() method |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first line of a method comment should always be in the third person: "[This method] Handles …"
* @param string $errStr | ||
* @param string $errFile | ||
* @param int $errLine | ||
* @param array $errContext |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we know more about the array, e.g., string[][] or int[][]?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
context it's just an array of object contains context variables from current scope
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we know anything about the types of the contained context variables? Or will they be mixed int, string, bool etc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, they will be arrays, objects and strings ... mixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, then "array" will suffice. Thanks for the info!
* | ||
* @throws \InvalidArgumentException | ||
*/ | ||
public function errorHandler($errNo, $errStr, $errFile, $errLine, $errContext) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add an "array" type hint to the last parameter.
* @param int $errLine | ||
* @param array $errContext | ||
* | ||
* @return bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure
* | ||
* @throws \InvalidArgumentException | ||
*/ | ||
public function errorHandler($errNo, $errStr, $errFile, $errLine, $errContext) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No abrreviations ("err"), please. Actually, I think that we don't need the prefix anyway as the context already is given by the method.
@@ -1501,4 +1505,34 @@ private function getNodesToExclude(\DOMXPath $xPath) | |||
|
|||
return $excludedNodes; | |||
} | |||
|
|||
/** | |||
* Handle warnings in process() method |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also add some description on what the method actually does.
*/ | ||
public function errorHandler($errNo, $errStr, $errFile, $errLine, $errContext) | ||
{ | ||
if ($errNo === E_WARNING && isset($errContext['cssRule']['selector'])) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this check, or will the handler also be called in other cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not really :) but
- code sniffer trigger error about not used variable
- errorHandler will be called even for E_ERRORS
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? |
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! |
Only way is skip this test in HHVM |
Okay, then we need to skip it depending on the runtime. Here is an example: sebastianbergmann/phpunit#1356 |
done, check latest version |
@OzzyCzech Thanks! I'll have a look at it! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks very good, but still needs some minor changes.
$this->process($xmlDocument); | ||
restore_error_handler(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can move handler code to process()
or add processWithErrorHandling()
method
|
||
/** | ||
* Handles invalid xPath expression warnings, generated by process() method, | ||
* during querying \DOMDocumentand and trigger \InvalidArgumentException |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a space missing before the "and".
/** | ||
* Handles invalid xPath expression warnings, generated by process() method, | ||
* during querying \DOMDocumentand and trigger \InvalidArgumentException | ||
* with invalid selector |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add the missing period at the end.
// 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'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be markTestSkipped, not markTestIncomplete. (markTestIncomplete serves as a kind of "to do" marker.)
* | ||
* @throws \InvalidArgumentException | ||
*/ | ||
public function handleXpathError($type, $message, $file, $line, array $context) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this method need to be public, or could it be private?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me is clear api better then have this function in public space
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. Then let's keep it this way. Thanks for the explanation!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall I leave it as is or hide callback to process?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please leave it as a public method. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:) ok, check latest version
* | ||
* @return void | ||
*/ | ||
private function processWithErrorHandling(\DOMDocument $xmlDocument) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, that can be part of process method
/** | ||
* Handles invalid xPath expression warnings, generated by process() method, | ||
* during querying \DOMDocument and trigger \InvalidArgumentException | ||
* with invalid selector |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add the missing period at the end of the sentence.
@@ -25,7 +25,12 @@ This project adheres to [Semantic Versioning](http://semver.org/). | |||
|
|||
### Security | |||
|
|||
## 1.2.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line should not be here. (We'll add it later for the release.)
|
||
### Added |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's also already an "Added" section (line 9) where you can add the feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am blind 🤦♂️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
;-)
@@ -329,6 +329,7 @@ public function emogrifyBodyContent() | |||
*/ | |||
protected function process(\DOMDocument $xmlDocument) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method now needs the "throws" annotation (and an explanation on the same line explaining that the Exception is thrown for invalid XPath).
@@ -402,6 +403,7 @@ protected function process(\DOMDocument $xmlDocument) | |||
} | |||
|
|||
$this->copyCssWithMediaToStyleNode($xmlDocument, $xPath, $cssParts['media']); | |||
restore_error_handler(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great. Thanks!
You are welcome! |
BC break:
It’s related to #360 issue