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

[BUGFIX] Remove uppercase !important from attributes #911

Merged
merged 4 commits into from
Jun 23, 2020
Merged

[BUGFIX] Remove uppercase !important from attributes #911

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1728d01
[BUGFIX] Remove uppercase !important from attributes
JakeQZ Jun 21, 2020
9ec49ff
Changes suggested in code review:
JakeQZ Jun 23, 2020
d90019a
Disable PHPCS rule which is broken...
JakeQZ Jun 23, 2020
6c48cba
Update CHANGELOG to reflect all fixes.
JakeQZ Jun 23, 2020
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
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,9 @@ This project adheres to [Semantic Versioning](https://semver.org/).
([#880](https://github.com/MyIntervals/emogrifier/pull/880))

### Fixed
- Remove `!important` from `style` attributes also when uppercase, mixed case or
having whitespace after `!`
([#911](https://github.com/MyIntervals/emogrifier/pull/911))
- Copy rules using `:...of-type` without a type to the `<style>` element
([#904](https://github.com/MyIntervals/emogrifier/pull/904))
- Support combinator followed by dynamic pseudo-class in minified CSS
Expand Down
6 changes: 5 additions & 1 deletion phpcs.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,11 @@
<exclude name="Squiz.Commenting.FunctionComment.ThrowsNotCapital"/>
<exclude name="Squiz.Commenting.FunctionComment.ThrowsNoFullStop"/>
</rule>
<rule ref="Squiz.Commenting.FunctionCommentThrowTag"/>
<rule ref="Squiz.Commenting.FunctionCommentThrowTag">
<!-- If we want to document exceptions thrown by called methods for a method that also throws its own
exceptions, this sniff simply does not work. -->
<exclude name="Squiz.Commenting.FunctionCommentThrowTag.WrongNumber"/>
</rule>
<rule ref="Squiz.Commenting.PostStatementComment"/>

<!-- Control structures -->
Expand Down
67 changes: 64 additions & 3 deletions src/CssInliner.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,8 @@ class CssInliner extends AbstractHtmlProcessor
*
* @return self fluent interface
*
* @throws ParseException
* @throws ParseException in debug mode, if an invalid selector is encountered
* @throws \RuntimeException in debug mode, if an internal PCRE error occurs
*/
public function inlineCss(string $css = ''): self
{
Expand Down Expand Up @@ -995,7 +996,7 @@ private function generateStyleStringFromDeclarationsArrays(array $oldStyles, arr
*/
private function attributeValueIsImportant(string $attributeValue): bool
{
return \strtolower(\substr(\trim($attributeValue), -10)) === '!important';
return (bool)\preg_match('/!\\s*+important$/i', $attributeValue);
}

/**
Expand All @@ -1019,6 +1020,8 @@ private function fillStyleAttributesWithMergedStyles(): void
/**
* Searches for all nodes with a style attribute and removes the "!important" annotations out of
* the inline style declarations, eventually by rearranging declarations.
*
* @throws \RuntimeException
*/
private function removeImportantAnnotationFromAllInlineStyles(): void
{
Expand All @@ -1036,6 +1039,8 @@ private function removeImportantAnnotationFromAllInlineStyles(): void
* to "font-size: 13px; font: 12px serif;" in order to remain correct.
*
* @param \DOMElement $node
*
* @throws \RuntimeException
*/
private function removeImportantAnnotationFromNodeInlineStyle(\DOMElement $node): void
{
Expand All @@ -1044,7 +1049,7 @@ private function removeImportantAnnotationFromNodeInlineStyle(\DOMElement $node)
$importantStyleDeclarations = [];
foreach ($inlineStyleDeclarations as $property => $value) {
if ($this->attributeValueIsImportant($value)) {
$importantStyleDeclarations[$property] = \trim(\str_replace('!important', '', $value));
$importantStyleDeclarations[$property] = $this->pregReplace('/\\s*+!\\s*+important$/i', '', $value);
Copy link
Contributor Author

@JakeQZ JakeQZ Jun 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Psalm says that preg_replace might return null. I think it's more likely that the server will crash and it won't return anything at all, but Psalm doesn't seem to cover that eventuality.

Please reject this change and either remove Psalm from the project or tell me how I'm supposed to actually make code changes that satisfy Psalm in some kind-of semi-positive way.

Copy link
Contributor

Choose a reason for hiding this comment

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

In this case, Psalm is right. preg_replace will return null if there is an error.

For doing this in a cleaner way, we could do something like this:
… = \preg_replace(…) ?? ''
This will return an empty string in case of an error.
Or we could throw an exception if null is returned to make the problem visible.

} else {
$regularStyleDeclarations[$property] = $value;
}
Expand Down Expand Up @@ -1289,4 +1294,60 @@ private function getHeadElement(): \DOMElement
{
return $this->domDocument->getElementsByTagName('head')->item(0);
}

/**
* Wraps `preg_replace`. If an error occurs (which is highly unlikely), either it is logged and the original
* `$subject` is returned, or in debug mode an exception is thrown.
*
* This method does not currently allow `$subject` (and return value) to be an array, because a means of telling
* Psalm that a method returns the same type a particular parameter has not been found (though it knows this for
* `preg_replace`); nor does it currently support the optional parameters.
*
* @param string|string[] $pattern
* @param string|string[] $replacement
* @param string $subject
*
* @return string
*
* @throws \RuntimeException
*/
private function pregReplace($pattern, $replacement, string $subject): string
{
$result = \preg_replace($pattern, $replacement, $subject);

if ($result === null) {
$this->logOrThrowPregLastError();
$result = $subject;
}

return $result;
}

/**
* Obtains the name of the error constant for `preg_last_error` (based on code posted at
* {@see https://www.php.net/manual/en/function.preg-last-error.php#124124}) and puts it into an error message
* which is either passed to `trigger_error` (in non-debug mode) or an exception which is thrown (in debug mode).
*
* @throws \RuntimeException
*/
private function logOrThrowPregLastError(): void
{
$pcreConstants = \get_defined_constants(true)['pcre'];
$pcreErrorConstantNames = \is_array($pcreConstants) ? \array_flip(\array_filter(
$pcreConstants,
function (string $key): bool {
return \substr($key, -6) === '_ERROR';
},
ARRAY_FILTER_USE_KEY
)) : [];

$pregLastError = \preg_last_error();
$message = 'PCRE regex execution error `' . (string)($pcreErrorConstantNames[$pregLastError] ?? $pregLastError)
. '`';

if ($this->debug) {
throw new \RuntimeException($message, 1592870147);
}
\trigger_error($message);
}
}
78 changes: 68 additions & 10 deletions tests/Unit/CssInlinerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2752,40 +2752,98 @@ private function buildSubjectWithBoilerplateHtml(string $style = ''): CssInliner
return $this->buildDebugSubject($html);
}

/**
* @return string[][]
*/
public function provideImportantDeclarationMarker(): array
{
return [
'lower case' => [' !important'],
'mixed case' => [' !ImPorTant'],
'upper case' => [' !IMPORTANT'],
'with space within' => [' ! important'],
'without space preceding' => ['!important'],
];
}

/**
* @test
*
* @param string $importantMarker
*
* @dataProvider provideImportantDeclarationMarker
*/
public function importantInExternalCssOverwritesInlineCss(): void
public function inlineCssRemovesImportantFromStyleAttribute(string $importantMarker): void
{
$subject = $this->buildSubjectWithBoilerplateHtml('margin: 2px;');
$subject = $this->buildSubjectWithBoilerplateHtml('margin: 1px' . $importantMarker . ';');

$subject->inlineCss('p { margin: 1px !important; }');
$subject->inlineCss();

self::assertStringContainsString('<p style="margin: 1px;">', $subject->renderBodyContent());
}

/**
* @test
*
* @param string $importantMarker
*
* @dataProvider provideImportantDeclarationMarker
*/
public function importantInExternalCssKeepsInlineCssForOtherAttributes(): void
public function inlineCssRemovesImportantFromInlinedStyleAttribute(string $importantMarker): void
{
$subject = $this->buildSubjectWithBoilerplateHtml('margin: 2px; text-align: center;');
$subject = $this->buildSubjectWithBoilerplateHtml();

$subject->inlineCss('p { margin: 1px !important; }');
$subject->inlineCss('p { margin: 1px' . $importantMarker . '; }');

self::assertStringContainsString('<p style="text-align: center; margin: 1px;">', $subject->renderBodyContent());
self::assertStringContainsString('<p style="margin: 1px;">', $subject->renderBodyContent());
}

/**
* @test
*
* @param string $importantMarker
*
* @dataProvider provideImportantDeclarationMarker
*/
public function importantIsCaseInsensitive(): void
public function importantInExternalCssOverwritesInlineCss(string $importantMarker): void
{
$subject = $this->buildSubjectWithBoilerplateHtml('margin: 2px;');

$subject->inlineCss('p { margin: 1px !ImPorTant; }');
$subject->inlineCss('p { margin: 1px' . $importantMarker . '; }');

self::assertStringContainsString('<p style="margin: 1px;">', $subject->renderBodyContent());
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we then also should rename the test.

Copy link
Contributor

Choose a reason for hiding this comment

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

And maybe we can use a data provider for the the important variants (lowercase, all-uppercase, mixed case).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And maybe we can use a data provider for the the important variants (lowercase, all-uppercase, mixed case).

Good idea. I have done that, and adjusted the tests. I also then found another bug - ! important (with a space after the !) was not being removed as intended (though it was being handled correctly elsewhere).

The final fix involves changing the str_replace not to a str_ireplace but to a preg_replace. Now Psalm complains because preg_replace might return null. So I have had to implement a wrapper to ensure that is not the case.

Do some of the other Psalm issues included in the baseline relate to this (we use preg_replace elsewhere)? Maybe this wrapper will allow that specific issue to be addressed in those other cases.

Copy link
Contributor Author

@JakeQZ JakeQZ Jun 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Seems I also need to disable a PHPCS rule, because I declared the extra exception that could be thrown. See squizlabs/PHP_CodeSniffer#2683.

}

/**
* @test
*
* @param string $importantMarker
*
* @dataProvider provideImportantDeclarationMarker
*/
public function importantInExternalCssNotOverwritesImportantInInlineCss(string $importantMarker): void
{
$subject = $this->buildSubjectWithBoilerplateHtml('margin: 2px' . $importantMarker . ';');

$subject->inlineCss('p { margin: 1px' . $importantMarker . '; }');

self::assertStringContainsString('<p style="margin: 2px;">', $subject->renderBodyContent());
}

/**
* @test
*
* @param string $importantMarker
*
* @dataProvider provideImportantDeclarationMarker
*/
public function importantInExternalCssKeepsInlineCssForOtherAttributes(string $importantMarker): void
{
$subject = $this->buildSubjectWithBoilerplateHtml('margin: 2px; text-align: center;');

self::assertStringContainsString('<p style="margin: 1px !ImPorTant;">', $subject->renderBodyContent());
$subject->inlineCss('p { margin: 1px' . $importantMarker . '; }');

self::assertStringContainsString('<p style="text-align: center; margin: 1px;">', $subject->renderBodyContent());
}

/**
Expand Down