Closes #3539 Combine @import rules into the minified CSS files

[engahmeds3ed]

Description

We added a layer to combine imported CSS files into the main minified/combined recursively.

Fixes #3539

Type of change

Please delete options that are not relevant.

• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update

Is the solution different from the one proposed during the grooming?

I added only a small part to handle rewrite paths and combine imports resursively.

How Has This Been Tested?

• Added new fixtures to be passed to unit/integration tests.
• Create page template with CSS file that has multiple imports and one of those imports has another import with some image paths and check if those are imported Also add another import for external file and it shouldn't be imported.

Checklist:

• My code follows the style guidelines of this project
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes
• Any dependent changes have been merged and published in downstream modules

[engahmeds3ed]

I created new filter here to allow importing external css files and it's disabled be default, you can find it here

wp-rocket/inc/Engine/Optimization/CSSTrait.php

Lines 361 to 369 in 43840d5

	/**
	* Filter Import external urls.
	*
	* @since 3.8.6
	*
	* @param bool Allow status.
	* @param string $file File path/URL.
	*/
	if ( (bool) apply_filters( 'rocket_allow_import_externals', false, $file ) ) {

[piotrbak]

@GeekPress @Tabrisrp Just a quick confirmation, we're all aligned that this kind of situation might break the layout, right?
@import internal.css
@import external.css

After the minification, internal.css rules will be below the external.css ones.

[GeekPress]

@piotrbak Yes we are aligned on that

[engahmeds3ed]

@wp-media/qa Ready for QA

[piotrbak]

@engahmeds3ed @Tabrisrp
For now, the import of the rules worked only for the absolute, internal path (4th point):
@import url('https://rocketlabsqa.ovh/wp-content/rocket-test-data/styles/import-css-fourth.css');

Tested scenarios:

1. @import url('import-css-third.css');
   Regular relative
2. @import url('import-css-second.min.css');
   Relative with .min.css
3. @import url('https://rocketlabsqa.ovh/wp-content/rocket-test-data/styles/import-css-fourth.css');
   Absolute internal
4. @import url("import-css-fifth.css");
   Inside import-css-fifth.css there's another import inside, used " instead of '
5. @import url('../../../wp-includes/css/dashicons.min.css');
   Relative path with ../../
6. @import url("https://www.w3schools.com/w3css/4/w3.css?ver=5.5.3");
   External path, used with the filter
7. @import url('import-css-eight.css?ver=1.2.3');
   Regular relative with query string
8. @import url("https://rocketlabsqa.dev/something.css");
   Stylesheet loading text/html instead of text/css
9. @import "import-css-ninth.css";
   Relative without url
10. @import url(import-css-tenth.css);
    Relative without " nor '
11. @import "import-css-eleventh.css" print;
    Relative without url with print
12. @import "import-css-twelfth.css" screen and (max-width: 10768px);
    Relative without url with media

Do you already know that we shouldn't cover any of the above scenarios? They're going to be tested on various of envs.

[remyperona]

@piotrbak

1. Yes it's fine, it's still an improvement as it will reduce HTTP requests, and minification will be applied on everything
2. I'm thinking we can start with a filter, and we'll see if we need to also enable it in the UI if we have requests for that (@engahmeds3ed)

[engahmeds3ed]

@Tabrisrp @piotrbak

Ok I will remove now the external filter and try to check the exclusion filter and verify if it's working or not and why.

[engahmeds3ed]

@piotrbak I removed external filter and also removed query strings from the css files to get contents properly and it works fine for me (please validate)

for the exclusion:

add_filter( 'rocket_skip_import_replacement', function( $skipped, $match_path ){
   if ( strstr( $match_path, 'file2.css' ) ) {
       return true;
    }
   return $skipped;
}, 10, 2 );

This code is working with me and I think I know why it wasn't working with u

Because this filter applies after moving relative paths to the new structure like
file2.css
is converted to
../../../../../css/file2.css
so we need to search with this path OR use regular expression to detect it.

so tomorrow morning I will adjust this code to accept array of files so support team would be able to use it.

[engahmeds3ed]

@wp-media/qa ready for another round of testing.

[piotrbak]

@engahmeds3ed Looks good on the single site, tomorrow I'm going to test on different environments + exclusions.

If I add .js file with @import, we're going to add the content of it to the css file. Shouldn't we make sure that the extension is .css?

[piotrbak]

@engahmeds3ed @Tabrisrp I tested on all our environments. The problem we're having now:

• If we add @charset "UTF-8"; at the very top, the external rules (ones that are not fetched) will be added before the charset, which is not correct.
  

It seems that the problem is also present on the trunk though:

To consider (that's an edge case, of course):

• Not importing files different than .css. The behaviour I'm experiencing on the not cached page:

1. This error in the JS console:
   Refused to apply style from '/wp-content/rocket-test-data/scripts/print-helloworld.js' because its MIME type ('application/javascript') is not a supported stylesheet MIME type, and strict MIME checking is enabled.
2. In the network tab it's canceled with 0 bytes downloaded:
   

[engahmeds3ed]

@piotrbak I think it's based on the content of the file, so in my case I created JS file with css contents inside it and I got this into netwrok tab

file1.css which is called into the page

file2.js which is imported into the previous file

[remyperona]

If it's based on the mime type, we could do a check on that before importing?

Or if it's too complicated, we can just exclude any import that doesn't have the .css extension, as this is really what is expected in that context, and anything different is not normal.

[GeekPress]

@piotrbak Have you ever see someone importing a .js into a CSS file? For which reason, a plugin/theme will do that?

I'm asking the question to know if we aren't trying to fox something which will never happen and/or a very rare case.

[piotrbak]

@GeekPress That's rather an edge case, I'm trying to find as many possible faults as possible before customers will do.

I've brought this up for discussion since it's possible that we'll have different behavior with/without WP Rocket:

1. Without, the import will fail and we'll not load the JS (based on this screenshot)
2. With, we'll add the JS to our CSS and we'll load it

[GeekPress]

That's rather an edge case, I'm trying to find as many possible faults as possible before customers will do.

@piotrbak I understand. But we shouldn't complexify our code to fix a mistake a user can do.

In this specific case, I can't see why a JS file could be added by a plugin / theme / something else.

[GeekPress]

From the definition of the @import:

The @import rule allows you to import a style sheet into another style sheet.

It clearly indicates that an @import is to import .css into another. How can a JavaScript file be used to import CSS rules?

[piotrbak]

@GeekPress You're totally right about this, and this hypothetical situation would be an obvious mistake of the developer or customer. My only concern is that without WP Rocket, the browser will prevent from downloading this file, with WP Rocket we'll download it.

The other scenarios passed and we'll be ready to merge.

[engahmeds3ed]

@Tabrisrp @piotrbak

firstly I used this WordPress function wp_check_filetype_and_ext that uses multiple ways to detect the file type and one of them is fileinfo module and when doing some tests I found the following bugs that may be in some servers (I found it also in my machine) which is mime-type not returns text/css and returns other unknown types.

https://forums.aws.amazon.com/thread.jspa?messageID=904363

so I used the WordPress function wp_check_filetype that checks the extension only.

[remyperona]

We can move forward with that, it's enough to validate.

[engahmeds3ed]

@piotrbak ready for QA

[piotrbak]

@Tabrisrp We're good there. Ready to merge:
testrail-report-139.pdf