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.

Sign up for GitHub

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

Jump to bottom

Fix InvalidArgumentException #392

Merged
merged 8 commits into from
Jun 30, 2017
Merged

Fix InvalidArgumentException #392

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
8f2e217
Fix InvalidArgumentException when xpath does not find the css selecto…
jriboux Jun 22, 2017
6736f56
Fix code format
jriboux-advisa Jun 22, 2017
3d1ee6f
Added debug mode. Throw debug exceptions only if debug is active.
jriboux-advisa Jun 23, 2017
872b901
Add missing pull request link in CHANGELOG.md
jriboux-advisa Jun 24, 2017
a69ee19
Enhance comments and annotations
jriboux-advisa Jun 24, 2017
5f6dae8
Remove unnecessary else in handleXpathError
jriboux-advisa Jun 24, 2017
d5bd8ed
Fix indentation
jriboux-advisa Jun 24, 2017
e2d8b69
[CLEANUP] Update the test names
oliverklee Jun 30, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ This project adheres to [Semantic Versioning](http://semver.org/).
## x.y.z (unreleased)

### Added

- Debug mode. Throw debug exceptions only if debug is active.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you also please link the PR (i.e., use the same format as for the other changelog entries)?


### Changed

Expand Down
39 changes: 30 additions & 9 deletions Classes/Emogrifier.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,13 @@ class Emogrifier
],
];

/**
* If true, allow debug Exception to be thrown.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's make this more specific: … Emogrifier will throw Exceptions when it encounters an error instead of silently ignoring them.

*
* @var bool
*/
private $debug = false;

/**
* The constructor.
*
Expand Down Expand Up @@ -1529,18 +1536,32 @@ private function getNodesToExclude(\DOMXPath $xPath)
public function handleXpathError($type, $message, $file, $line, array $context)
{
if ($type === E_WARNING && isset($context['cssRule']['selector'])) {
throw new \InvalidArgumentException(
sprintf(
'%s in selector >> %s << in %s on line %s',
$message,
$context['cssRule']['selector'],
$file,
$line
)
);
if ($this->debug) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need the else here as throwing an exception will exit the method anyway.

throw new \InvalidArgumentException(
sprintf(
'%s in selector >> %s << in %s on line %s',
$message,
$context['cssRule']['selector'],
$file,
$line
)
);
} else {
return false;
}
}

// the normal error handling continues when handler return false
return false;
}

/**
* Set debug mode.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please always use the third person for the first sentence of a method description: [This Method] "Sets the debug mode."

*
* @param bool $debug set to true to enable debug mode
*/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also add the return void annotation.

public function setDebug($debug)
{
$this->debug = $debug;
}
}
17 changes: 17 additions & 0 deletions Tests/Unit/EmogrifierTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -956,9 +956,26 @@ public function emogrifyForInvalidCssSelectorThrowsException()
'<html><style type="text/css">p{color:red;} <style data-x="1">html{cursor:text;}</style></html>'
);

$this->subject->setDebug(true);
$this->subject->emogrify();
}

/**
* @test
*/
public function emogrifyIgnoreInvalidCssSelectors()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also add another test that tests for the behavior without debug mode.

Copy link
Contributor Author

@jriboux jriboux Jun 24, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To test with debug mode enabled I kept the test from pull request #361 "Handling invalid xPath expression warnings" and added the following line to test debug mode :
$this->subject->setDebug(true);
See lines 948 to 961

The test emogrifyIgnoreInvalidCssSelectors it the test with debug mode disabled.

{
$html = '<html><style type="text/css">' .
'p{color:red;} <style data-x="1">html{cursor:text;} p{background-color:blue;}</style> ' .
'<body><p></p></body></html>';

$this->subject->setHtml($html);

$html = $this->subject->emogrify();
self::assertContains('color: red', $html);
self::assertContains('background-color: blue', $html);
}

/**
* Data provider for things that should be left out when applying the CSS.
*
Expand Down