@import rules aren't combined into the minified CSS files

[GeekPress]

Describe the bug
When the minify CSS option is turned on, the @import rules aren't combined into the current CSS file.

This is an important problem for performance:

• it creates useless HTTP requests
• The files needs to be preloaded to not decrease the PageSpeed Insight Grade

I did some tests on my side to know if the default behavior of the library was to combined @import rules.

The response is: YES

Please note that only internal @import rules are combined by the library. It means an @import rules from Google Fonts won't be merged.

I did a simple test on smashingcoding website. I minified this file which contains 2 @import rules:
http://smashingcoding.com/wp-content/plugins/forget-about-shortcode-buttons/public/css/button-styles.css

I used the last version of the library by using this code:

<?php
header('Content-type: text/css');
header('Cache-Control: max-age=31536000');

require_once 'minify/Minify.php';
require_once 'minify/CSS.php';
require_once 'path-converter/ConverterInterface.php';
require_once 'path-converter/Converter.php';

use MatthiasMullie\Minify;

$minifier = new Minify\CSS();
$minifier->add('wp-content/plugins/forget-about-shortcode-buttons/public/css/button-styles.css');

echo $minifier->minify();

The result is available here:
http://smashingcoding.com/minify.php

As you can notice the @import rules has been combined into the minified CSS file.

It means we have a problem into WP Rocket to be able to combine the @import rules.

/!\ The $maxImportSize variable doesn't have any relation with this problem. Increasing its value won't have any effect.

To Reproduce
Steps to reproduce the behavior:

1. Go to smashingcoding.com
2. Activate the Minify CSS option
3. Check the source code of the CSS file which starts by http://smashingcoding.com/wp-content/cache/min/1/wp-content/plugins/forget-about-shortcode-buttons/public/css/button-styles-
4. See error. The @import rules are still present

Expected behavior
The internal @import rules should be combined into the current CSS file.

Backlog Grooming (for WP Media dev team use only)

• Reproduce the problem
• Identify the root cause
• Scope a solution
• Estimate the effort

[engahmeds3ed]

efb7b07

this commit is prototype for the proposed solution

Branch name: prototype/fix-internal-import

Note: This change required me to add new method inside minify package here:

wp-rocket/inc/Dependencies/Minify/Minify.php

Lines 140 to 142 in efb7b07

	public function addData( $path, $content ) {
	$this->data[ $path ] = $content;
	}

also I removed changing urls for import from our side at class WP_Rocket\Engine\Optimization\CSSTrait method: move ( we may use it only for external urls )

@wp-media/php plz validate

[remyperona]

@engahmeds3ed Do you see a way of accomplishing the same goal, but without modifying the library code?

[engahmeds3ed]

@Tabrisrp

I think we have two options here:-

1. Pass the file path instead of content and then apply our filters after minification.
2. duplicated the minify package's code and implement again the part of replacing import by contents.

for now I opened PR at the minify package here: matthiasmullie/minify#359
I hope he can check that soon (in case we took this approach)

what do u think?

[engahmeds3ed]

@Tabrisrp I will try here to groom it by changing our code as it's major severity:-

Reproduce the problem
Yes I can see the issue locally and also our tests expect that behavior.

Identify the root cause
We don't send the source path with the content to minify package so it can't convert relative path to its absolute path then fails getting the import file contents.

Scope a solution
Change the following line

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

Line 47 in d1e429b

return apply_filters( 'rocket_css_content', $this->move( $this->get_converter( $source, $target ), $content, $source ), $source, $target );

to be

$content = $this->move( $this->get_converter( $source, $target ), $content, $source );

$content = $this->combine_imports( $content );

/**
 * Filters the content of a CSS file
 *
 * @since 3.4
 *
 * @param string $content CSS content.
 * @param string $source  Source filepath.
 * @param string $target  Target filepath.
 */
return apply_filters( 'rocket_css_content', $content, $source, $target );

Create the following method combine_imports on the same file

protected function combine_imports( $content ) {
	$importRegexes = array(
		// @import url(xxx)
		'/
		# import statement
		@import

		# whitespace
		\s+

			# open url()
			url\(

				# (optional) open path enclosure
				(?P<quotes>["\']?)

					# fetch path
					(?P<path>.+?)

				# (optional) close path enclosure
				(?P=quotes)

			# close url()
			\)

			# (optional) trailing whitespace
			\s*

			# (optional) media statement(s)
			(?P<media>[^;]*)

			# (optional) trailing whitespace
			\s*

		# (optional) closing semi-colon
		;?

		/ix',

		// @import 'xxx'
		'/

		# import statement
		@import

		# whitespace
		\s+

			# open path enclosure
			(?P<quotes>["\'])

				# fetch path
				(?P<path>.+?)

			# close path enclosure
			(?P=quotes)

			# (optional) trailing whitespace
			\s*

			# (optional) media statement(s)
			(?P<media>[^;]*)

			# (optional) trailing whitespace
			\s*

		# (optional) closing semi-colon
		;?

		/ix',
	);

	// find all relative imports in css
	$matches = array();
	foreach ($importRegexes as $importRegex) {
		if (preg_match_all($importRegex, $content, $regexMatches, PREG_SET_ORDER)) {
			$matches = array_merge($matches, $regexMatches);
		}
	}

	$search = array();
	$replace = array();

	// loop the matches
	foreach ($matches as $match) {
		// grab referenced file & minify it (which may include importing
		// yet other @import statements recursively)
		$importContent = $this->get_file_contents( $match['path'] );

		// check if this is only valid for certain media
		if (!empty($match['media'])) {
			$importContent = '@media '.$match['media'].'{'.$importContent.'}';
		}

		// add to replacement array
		$search[] = $match[0];
		$replace[] = $importContent;
	}

	// replace the import statements
	return str_replace($search, $replace, $content);
}

Add another method get_file_contents in the same file as follows:-

private function get_file_contents( $path ) {
	return rocket_direct_filesystem()->get_contents( $path );
}

Adjust the tests to accept the new behavior.

Question: this code will be applied before applying the minification itself so imports will be replaced by their contents so those imports won't get into top of the file as minification process won't see imports, I think this is the right behavior, if @wp-media/productrocket wants it to be at the top we will sort the matches on the previous code to be based on finding import keyword inside it.

Estimate the effort
[M]

[remyperona]

Previously, @import needed to be moved to the top, because they are not interpreted if that's not the case. We don't need that anymore, since they are replaced by the actual content.

Question: why create a new method get_file_contents(), when we already have one from the AbstractOptimization class?

$content = $this->move( $this->get_converter( $source, $target ), $content, $source );

$content = $this->combine_imports( $content );

Shouldn't those be in the reverse order? We will want to update paths in the content found in the import too.

[GeekPress]

Previously, @import needed to be moved to the top, because they are not interpreted if that's not the case. We don't need that anymore, since they are replaced by the actual content.

We will still have external @import not concern by this enhancement. Should we not keep the same behavior?

[engahmeds3ed]

@Tabrisrp

Question: why create a new method get_file_contents(), when we already have one from the AbstractOptimization class?

Good question, I created this new method as we are on trait and this trait can be used at any class so for current situation we can use AbstractOptimization one.

Shouldn't those be in the reverse order? We will want to update paths in the content found in the import too.

You are totally right, I think when writing this code we need to test that carefully as I'm getting the file contents based on the new converted file path, so we can check that on implementation.

@GeekPress

We will still have external @import not concern by this enhancement. Should we not keep the same behavior?

Don't worry about external ones, we didn't touch them and they will be on the top as is.