[BUGFIX] Remove uppercase !important from attributes #911
Conversation
| @@ -2785,7 +2785,7 @@ public function importantIsCaseInsensitive(): void | |||
|
|
|||
| $subject->inlineCss('p { margin: 1px !ImPorTant; }'); | |||
|
|
|||
| self::assertStringContainsString('<p style="margin: 1px !ImPorTant;">', $subject->renderBodyContent()); | |||
| self::assertStringContainsString('<p style="margin: 1px;">', $subject->renderBodyContent()); | |||
There was a problem hiding this comment.
I think we then also should rename the test.
There was a problem hiding this comment.
And maybe we can use a data provider for the the important variants (lowercase, all-uppercase, mixed case).
There was a problem hiding this comment.
And maybe we can use a data provider for the the
importantvariants (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.
There was a problem hiding this comment.
Seems I also need to disable a PHPCS rule, because I declared the extra exception that could be thrown. See squizlabs/PHP_CodeSniffer#2683.
- Use a data provider to provide various valid forms of `!important` (uppercase, lowercase, etc.) for testing; - Add tests confirming `!important` is removed from `style` attributes; - Remove `importantIsCaseInsensitive` test which is now superseded by the use of the above data provider; - Fix additional bug in which `! important` would not be removed from `style` attribute; The final item has meant replacing a `str_ireplace` with a `preg_replace`. Psalm picks up that in exceptional circumstances the latter may return `null`. Thus, a wrapper for `preg_replace` is required, which will not return `null`: - In debug mode it will throw an exception; - In non-debug mode it will log an error and return the original input string.
JakeQZ
force-pushed
the
bugfix/uppercase-important
branch
from
c6b3dcf to
9ec49ff
Compare
...if we want to document exceptions thrown by methods called by a method.
|
This turns out to be not quite as straightforward as I'd hoped. The PR is ready for re-review. However, if you'd like it split into separate PRs, I'd be both happy and loathed to do so ;) On the subject of whether |
| @@ -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); | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
No description provided.