[CLEANUP] Refactor `parseCssRules`: parse all CSS #528

JakeQZ · 2018-02-22T17:19:53Z

Refactored parseCssRules so that it can now parse an entire CSS string,
including @media rules. It now returns two arrays, in an array with the
following keys:

"inlineable" => The inlineable rules that would have been previously returned;
"uninlineable" => The uninlineable rules, currently those from within @media
rules.

The original line number information from the CSS is available in both arrays,
so the original ordering of all the rules (that will be required for #280) can
be determined.

A temporary method combineCssMediaRules has been added to recombine the @media
rules back into a string of CSS as expected by copyCssWithMediaToStyleNode,
pending refactoring of that method to instead accept a parsed rule array as its
parameter. This does mean that, for now, the rules within each @media rule are
parsed again.

Refactored `parseCssRules` so that it can now parse an entire CSS string, including @media rules. It now returns two arrays, in an array with the following keys: - "inlineable" => The inlineable rules that would have been previously returned; - "uninlineable" => The uninlineable rules, currently those from within @media rules. The original line number information from the CSS is available in both arrays, so the original ordering of all the rules (that will be required for MyIntervals#280) can be determined. A temporary method `combineCssMediaRules` has been added to recombine the @media rules back into a string of CSS as expected by `copyCssWithMediaToStyleNode`, pending refactoring of that method to instead accept a parsed rule array as its parameter. This does mean that, for now, the rules within each @media rule are parsed again.

JakeQZ · 2018-02-22T18:13:52Z

As you'll see, the code in combineCssMediaRules is somewhat messy and I think would benefit from a CssConcatenator class to abstract the functionality for combining rules with the same media query, selectors or declarations block. (This functionality will still be required after refactoring copyCssWithMediaToStyleNode, then by the refactored method itself.) This I propose to add in the next incremental PR, though it could be included in this PR if desired...

oliverklee · 2018-02-22T20:30:58Z

src/Emogrifier.php

+
+            $cssRules = [
+                'inlineable' => [],
+                'uninlineable' => []


Please add a trailing comma also after the last element of a multiline array. This makes appending elements possible without having to modify the existing line.

oliverklee · 2018-02-22T20:31:51Z

src/Emogrifier.php

-                    ];
+                    $selectors = explode(',', $cssRule[1]);
+                    foreach ($selectors as $selector) {
+                        // don't process pseudo-elements and behavioral (dynamic) pseudo-classes;


Can we have this part (within the foreach loop of $selector, or most of it) in a separate private method?

I've moved the outer loop and regex matching into a separate method, so the loops are now very similar to how they were prior to this PR.

oliverklee · 2018-02-22T20:32:37Z

src/Emogrifier.php

+                'uninlineable' => []
+            ];
+            // keep track of where each rule appears in the file, since order is important
+            $key = 0;


Can't we have this as the key in the corresponding loop?

Currently it needs to increment with the inner loop and carry on incrementing with each iteration of the outer loop. However, it should be possible to have one double-loop doing the regex matching and array_merge-ing the results, then a second single loop processing the matches...

Changed as per comment above.

oliverklee · 2018-02-22T20:33:36Z

src/Emogrifier.php

+     *
+     * @return string
+     */
+    private function combineCssMediaRules(array $cssRules)


Can you also please split up this method? It's far too long. This would also allow to get rid of the comments (if the methods are well-named).

This is why I'd like to add a CssConcatenator class.

Now using CssConcatenator.

oliverklee

Generally, I propose you do this kind of changes to one file first, and then do it to the other file once the changes to one file are final.

oliverklee · 2018-02-22T20:37:01Z

As for the CssConcatenator class, we will only be able to have this for the CssInliner class, not for Emogrifier anymore (as the Emogrifier class is Composer-optional, and the class will go away for version 4.0.0).

oliverklee · 2018-02-22T20:37:29Z

Oops, wrong button. Sorry, Reopening.

oliverklee

Could you also please rebase to resolve the merge conflicts?

JakeQZ · 2018-02-22T23:00:41Z

As for the CssConcatenator class, we will only be able to have this for the CssInliner class, not for Emogrifier anymore (as the Emogrifier class is Composer-optional, and the class will go away for version 4.0.0).

Would something like

        // support use without autoload
        if (!class_exists('Pelago\\Emogrifier\\CssConcatenator')) {
            require_once __DIR__ . '/Emogrifier/CssConcatenator.php';
        }

not be possible within the Emogrifier class? I understand the need to support those not using autoload, but is it also a requirement to support grabbing just the .php file rather than the src directory (tree)?

oliverklee · 2018-02-22T23:15:38Z

No, that would be okay for me.

JakeQZ · 2018-02-23T01:30:40Z

I've added a CssConcatenator class as a separate PR (#530). I'll come back to this one when that is agreed.

These will be merged as a separate PR once the equivalent changes to Emogrifier.php have been agreed.

- Moved the regex matching part of `parseCssRules` (along with outer loop) into a separate private method; - Added trailing comma in multiline array definition; - Use `CssConcatenator` in `combineCssMediaRules`.

JakeQZ · 2018-03-29T23:55:13Z

Now made the changes suggested in review. Also rebased and reverted changes to CssInliner.php which can be merged as a separate PR once agreed.

oliverklee

Looks very good. Just needs the finishing polish!

oliverklee · 2018-03-30T16:51:31Z

src/Emogrifier.php


            $this->caches[static::CACHE_KEY_CSS][$cssKey] = $cssRules;
        }

        return $this->caches[static::CACHE_KEY_CSS][$cssKey];
    }

+    /**
+     * Parse a string of CSS into the media query, selectors and declarations for each ruleset in order


Third person, please ("Parses") and a period at the end of sentence.

oliverklee · 2018-03-30T16:53:58Z

src/Emogrifier.php

+                    if ($cssRule['media'] === '') {
+                        $cssRules['inlineable'][] = $parsedCssRule;
+                    } else {
+                        $cssRules['uninlineable'][] = $parsedCssRule;


I think we can shorten this to something like this:

$ruleType = ($cssRule['media'] === '') ? 'inlineable' : 'uninlineable'; $cssRules[$ruleType] = $parsedCssRule;

oliverklee

I've just done the last two polishing tasks myself. Thanks for this refactoring, @JakeQZ! ❤️

oliverklee assigned JakeQZ Feb 22, 2018

oliverklee added the cleanup label Feb 22, 2018

oliverklee added this to the 2.1.0 milestone Feb 22, 2018

oliverklee added the in review label Feb 22, 2018

oliverklee reviewed Feb 22, 2018

View reviewed changes

oliverklee requested changes Feb 22, 2018

View reviewed changes

oliverklee closed this Feb 22, 2018

oliverklee reopened this Feb 22, 2018

oliverklee added waiting for feedback and removed in review labels Feb 22, 2018

oliverklee requested changes Feb 22, 2018

View reviewed changes

oliverklee added blocked and removed waiting for feedback labels Feb 26, 2018

oliverklee added waiting for feedback and removed blocked labels Mar 29, 2018

JakeQZ added 3 commits March 30, 2018 00:26

Merge branch 'master' into refactor/parseCssRules

59c31f0

Reverted changes to CssInliner.php

3ca3726

These will be merged as a separate PR once the equivalent changes to Emogrifier.php have been agreed.

Changes following review:

53c169f

- Moved the regex matching part of `parseCssRules` (along with outer loop) into a separate private method; - Added trailing comma in multiline array definition; - Use `CssConcatenator` in `combineCssMediaRules`.

oliverklee added the in review label Mar 30, 2018

oliverklee removed the waiting for feedback label Mar 30, 2018

oliverklee requested changes Mar 30, 2018

View reviewed changes

oliverklee added waiting for feedback and removed in review labels Mar 30, 2018

Update Emogrifier.php

5618067

oliverklee approved these changes Mar 31, 2018

View reviewed changes

oliverklee removed the waiting for feedback label Mar 31, 2018

oliverklee merged commit e043b5f into MyIntervals:master Mar 31, 2018

JakeQZ added a commit to JakeQZ/emogrifier that referenced this pull request Apr 1, 2018

[CLEANUP] Merge MyIntervals#528 into CssInliner

2cf9ddd

JakeQZ mentioned this pull request Apr 1, 2018

[CLEANUP] Merge #528 into CssInliner #538

Merged

oliverklee pushed a commit that referenced this pull request Apr 2, 2018

[CLEANUP] Merge #528 into CssInliner (#538)

a628d52

JakeQZ deleted the refactor/parseCssRules branch April 2, 2018 21:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CLEANUP] Refactor `parseCssRules`: parse all CSS #528

[CLEANUP] Refactor `parseCssRules`: parse all CSS #528

JakeQZ commented Feb 22, 2018

JakeQZ commented Feb 22, 2018

oliverklee Feb 22, 2018

JakeQZ Mar 29, 2018

oliverklee Feb 22, 2018

JakeQZ Mar 29, 2018

oliverklee Feb 22, 2018

JakeQZ Feb 22, 2018

JakeQZ Mar 29, 2018

oliverklee Feb 22, 2018 •

edited

Loading

JakeQZ Feb 22, 2018

JakeQZ Mar 29, 2018

oliverklee left a comment

oliverklee commented Feb 22, 2018

oliverklee commented Feb 22, 2018

oliverklee left a comment

JakeQZ commented Feb 22, 2018

oliverklee commented Feb 22, 2018

JakeQZ commented Feb 23, 2018

JakeQZ commented Mar 29, 2018

oliverklee left a comment

oliverklee Mar 30, 2018

oliverklee Mar 30, 2018

oliverklee left a comment

[CLEANUP] Refactor parseCssRules: parse all CSS #528

[CLEANUP] Refactor parseCssRules: parse all CSS #528

Conversation

JakeQZ commented Feb 22, 2018

JakeQZ commented Feb 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliverklee Feb 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliverklee left a comment

Choose a reason for hiding this comment

oliverklee commented Feb 22, 2018

oliverklee commented Feb 22, 2018

oliverklee left a comment

Choose a reason for hiding this comment

JakeQZ commented Feb 22, 2018

oliverklee commented Feb 22, 2018

JakeQZ commented Feb 23, 2018

JakeQZ commented Mar 29, 2018

oliverklee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliverklee left a comment

Choose a reason for hiding this comment

[CLEANUP] Refactor `parseCssRules`: parse all CSS #528

[CLEANUP] Refactor `parseCssRules`: parse all CSS #528

oliverklee Feb 22, 2018 •

edited

Loading