Prevent incorrectly capitalized CSS selectors from being stripped #85

jdchmiel · 2014-04-15T17:49:40Z

Some wysiwyg editors seem to generate terrible CSS such as:
P {
PADDING: 0PX;
MARGIN: 0PX;
}
which due to the capitalized P was not being applied inline.

oliverklee · 2014-04-15T19:00:51Z

Tests/Unit/EmogrifierTest.php

+        $this->subject->setHtml($html);
+
+        $result = $this->subject->emogrify();
+        $this->assertContains($expected, $result);


Could you please put the expected value and the actual value in separate lines each? (And if you like, you can inline $result.)

oliverklee · 2014-04-15T19:03:28Z

Hi, thanks for the pull request. Just two things:

a) for assertContains, the expected and the actual should be in separate lines each
b) Could you please prefix the commit message subject line with [BUGFIX] and make the first letter of the first word uppercase? Maybe you could also shorten the commit message to something like "[BUGFIX] Apply CSS attribute names in a case-insensitive way

jdchmiel · 2014-04-15T19:26:24Z

It will take me a bit of time to get back to this, but yes, I will do so.

oliverklee · 2014-04-15T19:29:58Z

Thanks! After you've re-pushed the pull request, could you please notify me here in the PR? GitHub does not notify me of re-pushes. :( Thanks!

jdchmiel · 2014-04-17T17:55:29Z

I am trying to figure out how to squash now.
..And I see the Travis error with preg_replace_callback...

oliverklee · 2014-04-17T18:13:42Z

git rebase -i, and then mark all commits except for the first one with "s".

oliverklee · 2014-04-17T18:14:40Z

Travis fails because the "e" modifier for preg_replace_callback is not allowed anymore in PHP 5.5.

jdchmiel · 2014-04-17T18:58:04Z

Classes/Emogrifier.php

@@ -315,7 +315,7 @@ function (array $m) {

        foreach ($this->caches[self::CACHE_KEY_CSS][$cssKey] as $value) {
            // query the body for the xpath selector
-            $nodesMatchingCssSelectors = $xpath->query($this->translateCssToXpath(trim($value['selector'])));
+            $nodesMatchingCssSelectors = $xpath->query($this->translateCssToXpath($value['selector']));


trim is done in translateCssToXpath

oliverklee · 2014-04-17T19:06:48Z

Could you also please add a unit tests that checks that the attribute values keep their case, e.g. content: "Hello world";?

jdchmiel · 2014-04-17T19:27:51Z

excellent point, it does undercase content. Is that the only exception or should we allow such things as:

margin:0PX 1pX 2Px 3px;

to pass through?

oliverklee · 2014-04-17T20:00:08Z

I'd say we leave all attribute values unchanged to keep things simple.

oliverklee · 2014-04-17T21:27:36Z

By the way, Jared, you can also run the unit tests and style checks on your local machine. This allows you to get quick feedback on your code without having to wait for Travis.

jdchmiel · 2014-04-18T15:44:05Z

This is my first exposure to travis, composer, and PHP Code sniffer.
I was running the commands I saw in the .travis.yml file, but that only works for the single version of PHP I have installed. I did not yet see if I can use composer to set up and specify additional PHP versions. The ; issue was me refactoring the format of the code and then forgetting to run the unit tests again. The preg_match /e issue is in php 5.5 which is odd as that dev box does have 5.5.3 but a zend version. I am still investigating that for curiosities sake. This is all peripheral work as time permits unfortunately, but I do appreciate your assistance and guidance to getting up to speed with this.

jdchmiel · 2014-04-22T21:13:20Z

I think my rebase is not working out due to already pushing and then rebasing with things I had pushed already. I am way past the point of comfort with the amount of 'change' in one commit now though..

oliverklee · 2014-04-23T06:55:39Z

Hi, you can use the --force option when pushing if you have pushed before to that remote branch.

jdchmiel · 2014-05-01T14:55:06Z

git reset --soft HEAD^
git push origin +master
I found this as the way to merge all of my commits into one.

oliverklee · 2014-05-03T16:05:12Z

Classes/Emogrifier.php

@@ -596,6 +595,13 @@ private function getCssSelectorPrecedence($selector) {
     * @return string
     */
    private function translateCssToXpath($cssSelector) {
+        $cssSelector = ' ' . $cssSelector . ' ';
+        $cssSelector = preg_replace_callback('/\s+\w+\s+/',
+            function($matches) {


Could you please add the array type hint for the parameter?

jdchmiel · 2014-06-18T12:37:27Z

I ended up making the semicolon in each rule have a trailling space as well as the colon as it seemed more consistent. Thoughts?

oliverklee · 2014-06-18T14:37:16Z

Sounds good.

oliverklee · 2014-06-18T15:37:54Z

Classes/Emogrifier.php

@@ -378,6 +363,25 @@ function (array $m) {
        }
    }

+
+    /**
+     * generateStyleStringFromDeclarationsArrays merges old or existing name/value array with new name/value array


generateStyleStringFromDeclarationsArrays -> This method

oliverklee · 2014-06-18T15:43:32Z

Classes/Emogrifier.php

+     * @return string
+     */
+    private function generateStyleStringFromDeclarationsArrays(array $oldStyles, array $newStyles) {
+        $combinedArray = array_merge($oldStyles, $newStyles);


Let's have semantic variable names instead of named that include the type:

combinedArray -> combinedStyles or oldAndNewStyles

oliverklee · 2014-06-18T15:50:07Z

Could you also please squash both commits into one?

…rcase attributes and selectors \#myID P { margin: 0px; } was not being applied correctly from style block p { MARGIN: 0PX; } was not being applied correctly from style block all inlined css from passed in css and parsed out <style> blocks is now lowercase [BUGFIX]Prevent incorrectly capitalized CSS selectors from being stripped Some wysiwyg editors seem to generate terrible CSS such as: P { PADDING: 0PX; MARGIN: 0PX; } which due to the capitalized P was not being applied inline. [BUGFIX] Apply CSS attribute names regardles of case, outputting lowercase attributes and selectors p { MARGIN: 0PX; } was not being applied correctly from style block all inlined css from passed in css and parsed out <style> blocks is now lowercase Prevent incorrectly capitalized CSS selectors from being stripped Some wysiwyg editors seem to generate terrible CSS such as: P { PADDING: 0PX; MARGIN: 0PX; } which due to the capitalized P was not being applied inline. fix my dumb replace deprecated preg_replace /e with preg_replace_callback to pass php 5.6 travis tests fix my dumb replace deprecated preg_replace /e with preg_replace_callback to pass php 5.6 travis tests Change undercase behavior to not change the case of the vss attribute value. Font names, content property (http://www.w3schools.com/cssref/pr_gen_content.asp) should be left alone. This fix exposes an inconsistency in the output CSS. Now all code paths output the css with no spacing around the : or ; which is breaking approx 25 unit tests that will be updated for the next commit. update the unit tests to expect no spaces in all resulting CSS instead of the mixed ouput based on what code path created it. update the regex to not skip when uppercase chars were present, added todo for possibly removing preg_match for less expensive explode. removed a few extra var_dumps I had left behind ran the .travis stuff manually to verify formatting / unit tests / lint and fixed formatting a bit, removed TODO but left comment in order to avoid generating a warning

jdchmiel · 2014-06-18T18:04:15Z

I stripped out the unicode char I had added to two of the unit tests as that cannot pass until my next pull request. - These were the two you asked about single vs double quotes.

Prevent incorrectly capitalized CSS selectors from being stripped

oliverklee · 2014-06-19T11:19:50Z

Looks good. Thanks for this change!

oliverklee reviewed Apr 15, 2014
View reviewed changes

jdchmiel reviewed Apr 17, 2014
View reviewed changes

jdchmiel mentioned this pull request Apr 17, 2014

inconsistent handling of css case #86

Closed

oliverklee reviewed May 3, 2014
View reviewed changes

jdchmiel closed this Jun 18, 2014

jdchmiel reopened this Jun 18, 2014

oliverklee reviewed Jun 18, 2014
View reviewed changes

oliverklee added a commit that referenced this pull request Jun 19, 2014

Merge pull request #85 from jdchmiel/master

17eb967

Prevent incorrectly capitalized CSS selectors from being stripped

oliverklee merged commit 17eb967 into MyIntervals:master Jun 19, 2014

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent incorrectly capitalized CSS selectors from being stripped #85

Prevent incorrectly capitalized CSS selectors from being stripped #85

jdchmiel commented Apr 15, 2014

oliverklee Apr 15, 2014

oliverklee commented Apr 15, 2014

jdchmiel commented Apr 15, 2014

oliverklee commented Apr 15, 2014

jdchmiel commented Apr 17, 2014

oliverklee commented Apr 17, 2014

oliverklee commented Apr 17, 2014

jdchmiel Apr 17, 2014

oliverklee commented Apr 17, 2014

jdchmiel commented Apr 17, 2014

oliverklee commented Apr 17, 2014

oliverklee commented Apr 17, 2014

jdchmiel commented Apr 18, 2014

jdchmiel commented Apr 22, 2014

oliverklee commented Apr 23, 2014

jdchmiel commented May 1, 2014

oliverklee May 3, 2014

jdchmiel commented Jun 18, 2014

oliverklee commented Jun 18, 2014

oliverklee Jun 18, 2014

oliverklee Jun 18, 2014

oliverklee commented Jun 18, 2014

jdchmiel commented Jun 18, 2014

oliverklee commented Jun 19, 2014

Prevent incorrectly capitalized CSS selectors from being stripped #85

Prevent incorrectly capitalized CSS selectors from being stripped #85

Conversation

jdchmiel commented Apr 15, 2014

oliverklee Apr 15, 2014

Choose a reason for hiding this comment

oliverklee commented Apr 15, 2014

jdchmiel commented Apr 15, 2014

oliverklee commented Apr 15, 2014

jdchmiel commented Apr 17, 2014

oliverklee commented Apr 17, 2014

oliverklee commented Apr 17, 2014

jdchmiel Apr 17, 2014

Choose a reason for hiding this comment

oliverklee commented Apr 17, 2014

jdchmiel commented Apr 17, 2014

oliverklee commented Apr 17, 2014

oliverklee commented Apr 17, 2014

jdchmiel commented Apr 18, 2014

jdchmiel commented Apr 22, 2014

oliverklee commented Apr 23, 2014

jdchmiel commented May 1, 2014

oliverklee May 3, 2014

Choose a reason for hiding this comment

jdchmiel commented Jun 18, 2014

oliverklee commented Jun 18, 2014

oliverklee Jun 18, 2014

Choose a reason for hiding this comment

oliverklee Jun 18, 2014

Choose a reason for hiding this comment

oliverklee commented Jun 18, 2014

jdchmiel commented Jun 18, 2014

oliverklee commented Jun 19, 2014