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

[BUGFIX] Remove all ‘unprocessable’ tags. #650

Merged
merged 1 commit into from
Feb 11, 2019
Merged

[BUGFIX] Remove all ‘unprocessable’ tags. #650

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
[BUGFIX] Remove all ‘unprocessable’ tags. 
Not all ‘unprocessable’ tags (`<wbr>` by default) were being removed, because a
‘live’ NodeList was being iterated.  This was a regression as a result of #627.

Also modified related tests to include a `<body>` element in the input HTML.
Prior to #627, `removeUnprocessableTags()` did not behave as documented – if
elements had content it would still remove the tags (though not the content).
The tests did not pick this up because `DOMDocument::loadHTML()` creates a
`<body>` element if there isn’t one, and if there is text content (at the start)
not inside any element, wraps it in a `<p>` element
(https://secure.php.net/manual/en/domdocument.loadhtml.php#88864).  (It does not
create a `<p>` element if there is already a `<body>` element.)
  • Loading branch information
@JakeQZ
JakeQZ committed Feb 3, 2019
commit 752c38d10617488a116034272838f291dc9070c7
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ This project adheres to [Semantic Versioning](https://semver.org/).
### Removed

### Fixed
- Remove all �unprocessable� (e.g. `<wbr>`) tags
([#650](https://github.com/MyIntervals/emogrifier/pull/650))
- Correct translated xpath of `:nth-child` selector
([#648](https://github.com/MyIntervals/emogrifier/pull/648))

Expand Down
6 changes: 5 additions & 1 deletion src/Emogrifier.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1518,7 +1518,11 @@ private function splitCssAndMediaQuery($css)
private function removeUnprocessableTags()
{
foreach ($this->unprocessableHtmlTags as $tagName) {
$nodes = $this->domDocument->getElementsByTagName($tagName);
// Deleting nodes from a 'live' NodeList invalidates iteration on it, so a copy must be made to iterate.
$nodes = [];
foreach ($this->domDocument->getElementsByTagName($tagName) as $node) {
$nodes[] = $node;
}
/** @var \DOMNode $node */
foreach ($nodes as $node) {
$hasContent = $node->hasChildNodes() || $node->hasChildNodes();
Expand Down
6 changes: 5 additions & 1 deletion src/Emogrifier/CssInliner.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1154,7 +1154,11 @@ private function splitCssAndMediaQuery($css)
private function removeUnprocessableTags()
{
foreach ($this->unprocessableHtmlTags as $tagName) {
$nodes = $this->domDocument->getElementsByTagName($tagName);
// Deleting nodes from a 'live' NodeList invalidates iteration on it, so a copy must be made to iterate.
$nodes = [];
foreach ($this->domDocument->getElementsByTagName($tagName) as $node) {
$nodes[] = $node;
}
/** @var \DOMNode $node */
foreach ($nodes as $node) {
$hasContent = $node->hasChildNodes() || $node->hasChildNodes();
Expand Down
25 changes: 20 additions & 5 deletions tests/Unit/CssInlinerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -369,12 +369,27 @@ public function renderNotAddsSecondContentTypeMetaTag()
static::assertSame(1, $numberOfContentTypeMetaTags);
}

/**
* @return string[][]
*/
public function wbrTagDataProvider()
{
return [
'single <wbr> tag' => ['<body>foo<wbr/>bar</body>'],
'two sibling <wbr> tags' => ['<body>foo<wbr/>bar<wbr/>baz</body>'],
'two non-sibling <wbr> tags' => ['<body><p>foo<wbr/>bar</p><p>bar<wbr/>baz</p></body>'],
];
}

/**
* @test
*
* @param string $html
*
* @dataProvider wbrTagDataProvider
*/
public function emogrifyByDefaultRemovesWbrTag()
public function emogrifyByDefaultRemovesWbrTag($html)
{
$html = '<html>foo<wbr/>bar</html>';
$subject = $this->buildDebugSubject($html);

$result = $subject->emogrify();
Expand All @@ -387,7 +402,7 @@ public function emogrifyByDefaultRemovesWbrTag()
*/
public function addUnprocessableTagRemovesEmptyTag()
{
$subject = $this->buildDebugSubject('<html><p></p></html>');
$subject = $this->buildDebugSubject('<body><p></p></body>');

$subject->addUnprocessableHtmlTag('p');
$result = $subject->emogrify();
Expand All @@ -400,7 +415,7 @@ public function addUnprocessableTagRemovesEmptyTag()
*/
public function addUnprocessableTagNotRemovesNonEmptyTag()
{
$subject = $this->buildDebugSubject('<html><p>foobar</p></html>');
$subject = $this->buildDebugSubject('<body><p>foobar</p></body>');

$subject->addUnprocessableHtmlTag('p');
$result = $subject->emogrify();
Expand All @@ -413,7 +428,7 @@ public function addUnprocessableTagNotRemovesNonEmptyTag()
*/
public function removeUnprocessableHtmlTagKeepsTagAgainAgain()
{
$subject = $this->buildDebugSubject('<html><p></p></html>');
$subject = $this->buildDebugSubject('<body><p></p></body>');

$subject->addUnprocessableHtmlTag('p');
$subject->removeUnprocessableHtmlTag('p');
Expand Down
25 changes: 20 additions & 5 deletions tests/Unit/EmogrifierTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -341,12 +341,27 @@ public function emogrifyNotAddsSecondContentTypeMetaTag()
static::assertSame(1, $numberOfContentTypeMetaTags);
}

/**
* @return string[][]
*/
public function wbrTagDataProvider()
{
return [
'single <wbr> tag' => ['<body>foo<wbr/>bar</body>'],
'two sibling <wbr> tags' => ['<body>foo<wbr/>bar<wbr/>baz</body>'],
'two non-sibling <wbr> tags' => ['<body><p>foo<wbr/>bar</p><p>bar<wbr/>baz</p></body>'],
];
}

/**
* @test
*
* @param string $html
*
* @dataProvider wbrTagDataProvider
*/
public function emogrifyByDefaultRemovesWbrTag()
public function emogrifyByDefaultRemovesWbrTag($html)
{
$html = '<html>foo<wbr/>bar</html>';
$this->subject->setHtml($html);

$result = $this->subject->emogrify();
Expand All @@ -359,7 +374,7 @@ public function emogrifyByDefaultRemovesWbrTag()
*/
public function addUnprocessableTagRemovesEmptyTag()
{
$this->subject->setHtml('<html><p></p></html>');
$this->subject->setHtml('<body><p></p></body>');

$this->subject->addUnprocessableHtmlTag('p');
$result = $this->subject->emogrify();
Expand All @@ -372,7 +387,7 @@ public function addUnprocessableTagRemovesEmptyTag()
*/
public function addUnprocessableTagNotRemovesNonEmptyTag()
{
$this->subject->setHtml('<html><p>foobar</p></html>');
$this->subject->setHtml('<body><p>foobar</p></body>');

$this->subject->addUnprocessableHtmlTag('p');
$result = $this->subject->emogrify();
Expand All @@ -385,7 +400,7 @@ public function addUnprocessableTagNotRemovesNonEmptyTag()
*/
public function removeUnprocessableHtmlTagKeepsTagAgainAgain()
{
$this->subject->setHtml('<html><p></p></html>');
$this->subject->setHtml('<body><p></p></body>');

$this->subject->addUnprocessableHtmlTag('p');
$this->subject->removeUnprocessableHtmlTag('p');
Expand Down