Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove LazyLoad CSS Background Images script from not processed pages #6133

Closed
piotrbak opened this issue Sep 1, 2023 · 1 comment · Fixed by #6144
Closed

Remove LazyLoad CSS Background Images script from not processed pages #6133

piotrbak opened this issue Sep 1, 2023 · 1 comment · Fixed by #6144
Assignees
Labels
effort: [XS] < 1 day of estimated development time module: lazyload background css images priority: medium Issues which are important, but no one will go out of business. type: enhancement Improvements that slightly enhance existing functionality and are fast to implement
Milestone

Comments

@piotrbak
Copy link
Contributor

piotrbak commented Sep 1, 2023

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 there's no WP Rocket buffer involved in processing the page, we should not inject the script

To Reproduce
Steps to reproduce the behavior:

  1. Enable LazyLoad CSS Background Images feature
  2. Add define('DONOTCACHEPAGE', true) to wp config
  3. Go to the website in incognito mode and check the source
  4. The script is there

Expected behavior
When buffer is not involved, the script should not be added

Acceptance Criteria (for WP Media team use only)

  1. The script should not be injected when define('DONOTCACHEPAGE', true) is set
  2. The script should not be injected when the page is excluded from being processed (never cache URLs, or from the page editor point of view)
  3. The script should be added when the cache is not generated but optimisations are there (add_filter( 'do_rocket_generate_caching_files', '__return_false' );)
  4. The same goes for LazyLoad Images script, no regression here
  5. The script should be added in regular circumstances
@piotrbak piotrbak added type: enhancement Improvements that slightly enhance existing functionality and are fast to implement priority: medium Issues which are important, but no one will go out of business. needs: grooming module: lazyload background css images labels Sep 1, 2023
@CrochetFeve0251
Copy link
Contributor

CrochetFeve0251 commented Sep 4, 2023

Reproduce the problem

The issue was easy to reproduce.

Identify the root cause

The root cause is that we are not checking DONOTCACHEPAGE constant inside should_lazyload from CanLazyloadTrait.

Scope a solution

To solve that we will have to check the DONOTCACHEPAGE at this level inside should_lazyload from CanLazyloadTrait.

To support excluded URI we can use the following logic inside the same method:

		global $wp;

		if ( is_user_logged_in() ) {
			return false;
		}

		$url = home_url( add_query_arg( [], $wp->request ) );
		$cache_reject_uri            = get_rocket_cache_reject_uri();
                if (! preg_match( "@$cache_reject_uri$@m", $url ) ) {
				return false;
			}

Then tests concerning lazyload will have to be modified and for that the code inside the lazyload not merge will help.

Estimate the effort

Effort XS

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort: [XS] < 1 day of estimated development time module: lazyload background css images priority: medium Issues which are important, but no one will go out of business. type: enhancement Improvements that slightly enhance existing functionality and are fast to implement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants