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

Add a notification when Mod Pagespeed is enabled and tell customers that it is likely to conflict with WP Rocket #3369

Closed
DahmaniAdame opened this issue Nov 26, 2020 · 27 comments · Fixed by #3390
Assignees
Labels
3rd party compatibility Issues related to 3rd party compatibility like theme, plugin or hosting effort: [S] 1-2 days of estimated development time module: cache 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

@DahmaniAdame
Copy link
Contributor

DahmaniAdame commented Nov 26, 2020

Is your feature request related to a problem? Please describe.
Add a notification when Mod Pagespeed is enabled and tell customers that it is likely to conflict with WP Rocket, and that they should turn it off on their server to avoid any issues.

Describe the solution you'd like
We can check if Mod Pagespeed is enabled, maybe using the response header they send, and show notification on WP Rocket dashboard.

Describe alternatives you've considered
N/A

Additional context
Mod Pagespeed adds the following headers:

  • X-Mod-Pagespeed for Apache
  • X-Page-Speed for Nginx
@DahmaniAdame
Copy link
Contributor Author

@arunbasillal
Copy link
Contributor

Do we know why we have the conflict? Maybe we can work towards making WP Rocket compatible with mod_pagespeed rather than being incompatible with it.

cc: @sandyfigueroa @Tabrisrp

@arunbasillal arunbasillal added 3rd party compatibility Issues related to 3rd party compatibility like theme, plugin or hosting module: cache priority: medium Issues which are important, but no one will go out of business. type: bug Indicates an unexpected problem or unintended behavior waiting for feedback labels Nov 27, 2020
@Tabrisrp
Copy link
Contributor

There is a lot of features duplication between WP Rocket and the pagespeed module. Working toward being compatible would be like working to be compatible with W3 Total Cache or Swift Performance.

@arunbasillal
Copy link
Contributor

Thanks for the feedback Remy 👍 Makes sense.

@vmanthos
Copy link
Contributor

On a ticket I worked on, I had to exclude the following to prevent a jQuery not defined error:
/wp-includes/js/jquery/jquery.js,(.*).js

The URL of the jQuery file was like this:
https://www.example.com/wp-includes/js/jquery/jquery.js,qver=1.12.4-wp.pagespeed.jm.gp20iU5FlU.js

What do you think about at least excluding that pattern in the core?

screenshot

Related ticket: https://secure.helpscout.net/conversation/1335093437/210973?folderId=2135277

@arunbasillal
Copy link
Contributor

@vmanthos If we are going ahead with the notice suggesting to disable Mod Pagespeed, then is this still required? Or is this one of the most common issues where some users could still get by without disabling mod_pagespeed?

@vmanthos
Copy link
Contributor

vmanthos commented Dec 3, 2020

My comment was out of context. I apologize.

If we are going ahead with the notice suggesting to disable Mod Pagespeed, then is this still required?

In such a case, it won't be. 🙏

@engahmeds3ed engahmeds3ed added GROOMING IN PROGRESS Use this label when the issue is currently being groomed. and removed needs: grooming labels Dec 4, 2020
@engahmeds3ed
Copy link
Contributor

Grooming

Scope a solution ✅

  1. Create new thirdparty compatibility class here: inc/ThirdParty/Plugins/PageSpeed.php

  2. Create new method to check if pagespeed is enabled as follows:-

private static function has_pagespeed () {
	if ( function_exists( "apache_get_modules" ) ) {
		if ( in_array("mod_pagespeed", apache_get_modules(), true ) ) {
			return true;
		}
		return false;
	}

	if ( function_exists( "get_headers" ) ) {
		if ( ! empty( $_SERVER["HTTP_HOST"] ) ) {
			if ( ! empty( $headers = get_headers(home_url() ?? null ) ) ) {
				if ( ! empty( $headers["X-Mod-Pagespeed"] ) || ! empty( $headers["X-Page-Speed"] ) ) {
					return true;
				}
			}
		}
	}

	return false;
}
  1. create new callback for this action admin_notices to show the message.

Estimate the effort ✅

[S]

@wp-media/productrocket we need the message that will appear to the customer.

@engahmeds3ed engahmeds3ed added effort: [S] 1-2 days of estimated development time and removed GROOMING IN PROGRESS Use this label when the issue is currently being groomed. labels Dec 4, 2020
@engahmeds3ed
Copy link
Contributor

@wp-media/productrocket @arunbasillal we have two questions please:-

  1. Where is the message to be displayed?

  2. Do we need to show the notice only at WP Rocket settings page or in all pages?

Thanks.

@arunbasillal
Copy link
Contributor

@engahmeds3ed @Tabrisrp

  • Can we use the same notice where we recommend to deactivate non-compatible plugins?
  • Instead of the deactivate button, we could link it to a doc? (The button says Deactivate and a doc opens up)

Is this possible? If yes, I will have a chat with Jonathan and confirm.

@arunbasillal arunbasillal added this to the 3.8.2 milestone Dec 21, 2020
@engahmeds3ed
Copy link
Contributor

@wp-media/productrocket so the message will be like that:

WP Rocket: The following modules are not compatible with this plugin and may cause unexpected results:

Please confirm.

@GeekPress
Copy link
Contributor

@arunbasillal @webtrainingwheels Is it the expected doc to link?

@arunbasillal
Copy link
Contributor

@GeekPress @webtrainingwheels I think we could create a specific documentation for the same. Or maybe a separate section in the doc with an #anchor text.

Please advise Lucy.

@arunbasillal
Copy link
Contributor

Had a quick chat with Ahmed and he said that we are not reusing rocket_plugins_to_deactivate(), and instead we are doing a new notice. (For reference, I had asked if this was possible to re-use - #3369 (comment))

@webtrainingwheels In that case, isn't it better if we simply have a notice as below instead of - #3369 (comment)

WP Rocket: Mod PageSpeed is not compatible with this plugin and may cause unexpected results. More info

@webtrainingwheels
Copy link

@arunbasillal @GeekPress
In our current docs we mention mod pagespeed in 2 places:

We could make a dedicated doc but what message/info would we like to convey? 🤔

@arunbasillal
Copy link
Contributor

@webtrainingwheels

We could make a dedicated doc but what message/info would we like to convey?

  • I think a dedicated doc is better because it can be confusing otherwise when a user clicks from the notice. They will see the other titles and might not be sure if they landed in the right place.
  • The doc is basically a simple one that says it's not compatible with our features and a link to how to disable it. Just as we do now (with the external link). I don't think it needs much more than that.
  • We could reassure them that the PageSpeed module is not needed when they are using WP Rocket.

@engahmeds3ed
Copy link
Contributor

engahmeds3ed commented Dec 24, 2020

@wp-media/productrocket

image
This is the message view after adding it ( only waiting for docs url )

also I need to validate another point here,

check the following comment: #3390 (comment)
we are saving the status of mod_pagespeed existence at transient for 1 day so we will make this check every day, I added this 1 day from my mind and only wanted to validate if this will make any problem with our customers or not?

@webtrainingwheels
Copy link

@engahmeds3ed Here is the doc link: https://docs.wp-rocket.me/article/1376-mod-pagespeed

@arunbasillal
Copy link
Contributor

we are saving the status of mod_pagespeed existence at transient for 1 day so we will make this check every day, I added this 1 day from my mind and only wanted to validate if this will make any problem with our customers or not?

@engahmeds3ed Can we make the notice dismissible instead or it popping up every day? If users do not see a conflict with WP Rocket and wants to keep mod_pagespeed, they can simply dismiss the notice and never see it again. I think that would be better. If the notice pops up every day, we are definitely going to have users asking for an option to disable it.

@engahmeds3ed
Copy link
Contributor

@arunbasillal It's already like that so the clients can dismiss the message not to see it again.
I mean that if message is shown and then client removed mod_pagespeed so the message will disappear the next day or he can dismiss it for sure.

@arunbasillal
Copy link
Contributor

@engahmeds3ed To make sure I understand correctly:

  • User dismissed the notice
  • User decides to keep mod_pagespeed

In this case the notice is never displayed anymore, right?

(sorry for double checking)

@engahmeds3ed
Copy link
Contributor

Yes it will be like that one:
image

and when clicking on Dismiss this notice it will be removed for this user for ever.
I will check with you on slack when you are available.

@arunbasillal
Copy link
Contributor

@engahmeds3ed That looks neat to me 👍

@arunbasillal arunbasillal added type: enhancement Improvements that slightly enhance existing functionality and are fast to implement and removed type: bug Indicates an unexpected problem or unintended behavior labels Jan 6, 2021
@Tabrisrp Tabrisrp closed this as completed Jan 6, 2021
@twobyte
Copy link

twobyte commented Feb 18, 2021

Just stumbled across this message in WP Rocket, and after disabling mod_pagespeed on the server, our PageSpeed Insight results have regressed.

I think this message should explicitly state which specific features are not compatible, so the user can decide if they would like them managed at the application or server level.

Could some of these optimisations actually be better managed at the server level?

@GeekPress
Copy link
Contributor

Could some of these optimisations actually be better managed at the server level?

I can't see which feature done by mod_pagespeed could be better than what another tool like WP Rocket does.

@twobyte
Copy link

twobyte commented Mar 9, 2021

@GeekPress Even so, the existing message is very vague (unexpected results!).

-The only 100% conflict I'm aware of is with optimize css delivery and we mention it here:
https://docs.wp-rocket.me/article/1321-critical-css-issues-fouc

If this statement by @webtrainingwheels is correct, then many may find it easier to just disable critical CSS in WP Rocket, than perform server administration uninstalling mod_pagespeed.

@GeekPress
Copy link
Contributor

@twobyte Critical CSS is too much important to be disabled. Please correct, but mod_pagespeed doesn't do CPCSS as it should be done correctly.

By the way, mod_pagespeed isn't maintained since 2015. We will never recommend using an abandoned tool.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3rd party compatibility Issues related to 3rd party compatibility like theme, plugin or hosting effort: [S] 1-2 days of estimated development time module: cache 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.

8 participants