LazyLoad for CSS background images triggers DOMException errors when the CSS is coming from a PHP file

[sandyfigueroa]

Before submitting an issue please check that you’ve completed the following steps:

• Made sure you’re on the latest version ✅
• Used the search feature to ensure that the bug hasn’t been reported before ✅

Describe the bug
When a stylesheet is added from a PHP file, like this:

<link rel="stylesheet" id="custom-php-css-2" href="http://wp-rocket.esy.es/wp-content/throw-css.php" media="all">

The following error is thrown: DOMException: Failed to execute 'querySelectorAll' on 'Document': 'echo '.my-test-background-1' is not a valid selector.

It seems that the feature is getting the wrong selector, including the "echo '" in the PHP file.

Causing the querySelectorAll function to fail.

This error prevents all the Background images to be loaded, not only those coming from the PHP file.

To Reproduce
Steps to reproduce the behavior:

1. Add an element (a div for example) with a selector like ".my-test-background-1"

<div class="my-test-background-1"></div>

2. Throw a CSS from PHP. This is the code I used to test:

<?php
header('Content-Type: text/css');
echo '.my-test-background-1 {
    width: 500px;
    height: 500px;
    margin-left: auto;
    margin-right: auto;
    background-image: url("http://wp-rocket.esy.es/wp-content/uploads/2020/10/rose-165819_1920-1024x678.jpg");
    }'; 

3. Enable the LazyLoad for CSS background images
4. Clear WP Rocket's cache
5. Check the page

Expected behavior
The feature should get the correct selector to prevent this issue.

Acceptance Criteria (for WP Media team use only)
Clear instructions for developers, to be added before the grooming

[Miraeld]

Reproduce the problem

Like explain in the issue itself,
We can create a page with the following HTML:

<div class="my-test-background-1"></div>

Creating a test.php and put it in the rocket-test-data folder:

<?php
header('Content-Type: text/css');
echo '.my-test-background-1 {
    width: 500px;
    height: 500px;
    margin-left: auto;
    margin-right: auto;
    background-image: url("http://wp-rocket.esy.es/wp-content/uploads/2020/10/rose-165819_1920-1024x678.jpg");
    }'; 

And adding this to functions.php:

wp_enqueue_block_style(
    'test',
    array(
        'handle' => 'my-test-background-1',
        'src'    => get_home_url( null,'wp-content/rocket-test-data/styles/test.php' ),
        'path'   => get_home_url( null,'wp-content/rocket-test-data/styles/test.php' )
    )
);

We get the console error as mentioned.

Identify the root cause

The issue is that when we are extracting the content of the file, we keep the whole php content. Which means we are keeping the echo element.

Scope a solution

To solve the issue, we could modify the function

wp-rocket/inc/Engine/Media/Lazyload/CSS/Front/ContentFetcher.php

Line 58 in 46bc4ab

protected function fetch_content( string $path ) {

in order to check the file extension and if this is a php file, we could load the php to get its content instead of getting the file content.

	protected function fetch_content( string $path ) {
		if ( ! wp_http_validate_url( $path ) ) {
			$file_extension = pathinfo($path, PATHINFO_EXTENSION);

			if (strtolower($file_extension) !== 'php') {
				return $this->filesystem->get_contents($path);
			}
		}
		return wp_remote_retrieve_body( wp_remote_get( $path ) );
	}

would work.

Development steps:

Effort estimation:

XS

Is a refactor needed in that part of the codebase?

No

[jeawhanlee]

@Miraeld This solution would work fine if we had only echo to worry about but if we have cases like:

$script = '.my-test-background-1 {
   width: 500px;
   height: 500px;
   margin-left: auto;
   margin-right: auto;
   background-image: url("http://wp-rocket.esy.es/wp-content/uploads/2020/10/rose-165819_1920-1024x678.jpg");
   }'; 
   echo $script;

or

print '.my-test-background-1 {
    width: 500px;
    height: 500px;
    margin-left: auto;
    margin-right: auto;
    background-image: url("http://wp-rocket.esy.es/wp-content/uploads/2020/10/rose-165819_1920-1024x678.jpg");
    }'; 

to say the least, this kind of construct will trigger yet another console error.
could be an edge case though maybe @piotrbak

[Miraeld]

I thought about that, but I wan't able to think of a global solution for this kind of thing except playing with regex ... If you have an idea in mind, please share it

[jeawhanlee]

@wp-media/engineering-plugin-team I see here that we will get content here when the css url does not contain a host, so for this maybe we can do a check for a php ext and do a request with a constructed url to get the file buffer instead of the content.

[Miraeld]

Based on @jeawhanlee comment, here is what we could do:

In the function

wp-rocket/inc/Engine/Media/Lazyload/CSS/Front/ContentFetcher.php

Line 58 in 46bc4ab

protected function fetch_content( string $path ) {

we can add a condition to check if this is a php file, and if this is, then we return the content of the loaded php:

	protected function fetch_content( string $path ) {
		if ( ! wp_http_validate_url( $path ) ) {
			$file_extension = pathinfo($path, PATHINFO_EXTENSION);

			if (strtolower($file_extension) !== 'php') {
				return $this->filesystem->get_contents($path);
			}
		}
		return wp_remote_retrieve_body( wp_remote_get( $path ) );
	}

This would work.

I have updated the grooming accordingly

[Tabrisrp]

Looks good to me