Closes #5848: Elementor templates clearing cache in full

[jeawhanlee]

Description

Fixes full cache clearing when updating elementor templates. Edit: We added the logic to support conditional display. Fixes #5848

Type of change

• [x] Enhancement (non-breaking change which improves an existing functionality)

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

No

How Has This Been Tested?

• [ ] Automated Test
• [ ] Test locally

Checklist:

• [x] My code follows the style guidelines of this project
• [x] I have performed a self-review of my own code
• [x] I have commented my code, particularly in hard-to-understand areas
• [x] 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

[jeawhanlee]

@wp-media/php At the state of this PR we are not clearing cache for related posts when elementor renders templates based on set conditions. Do you see a simple and efficient way of getting related posts for conditional display of template in elementor aside the grooming here?

[piotrbak]

@wp-media/php Could you take a look at @jeawhanlee question?

[remyperona]

I don't think there is a simple way to handle those. Maybe these could be handled separately?

[piotrbak]

@jeawhanlee Could we also include clearing the Used CSS (if enabled) of the related posts here? https://github.com/wp-media/wp-rocket/issues/5848#issuecomment-1604048454

[jeawhanlee]

@jeawhanlee Could we also include clearing the Used CSS (if enabled) of the related posts here? #5848 (comment)

@piotrbak Yes we can

[vmanthos]

As #5848 is currently blocked, I'm adding the following note for the future:

We expect rocket_clean_post() to return a boolean, however rocket_pre_clean_post can return various values. It should be safeguarded from non-boolean values, e.g. strings, arrays, etc.

[vmanthos]

@jeawhanlee @piotrbak I looked into this.

We aren't clearing the whole cache when updating Elementor's templates.

However, when using a popup that:

1. is displayed site-wide
2. is updated

we don't clear the whole cache.

Also, if the popup is set to display only on posts, we won't clear their cache after it has been updated.

The same goes when changing the "display conditions" or the "triggers" that respectively affect where the popup should be displayed and when:

For example, if the user changes the "display conditions" from site-wide to a specific PT or vice versa we'd need to clear the whole cache for the changes to be reflected to the front-end.

I believe we should revisit this and carefully rewrite the acceptance criteria.

[MathieuLamiot]

@piotrbak Following @vmanthos feedbacks, it looks like we need to sort out more precisely the use-cases. Without this, we'll be iterating blindly. I am not sure who is more relevant to build the use-case list here, product or engineering? Based on this, would you be OK to take back this ticket to the backlog, or to "Needs Grooming"?

[piotrbak]

@MathieuLamiot Yes, it needs to go back to grooming. I'll build the list of use-cases today.

[remyperona]

Can you fix the following in the WP_Post fixture: Creation of dynamic property WP_Post::$post_type is deprecated

[vmanthos]

@jeawhanlee If I understand the acceptance criteria(AC) correctly, (@piotrbak correct me if I'm wrong), the button that is displayed when:

1. Remove Unused CSS is enabled AND
2. An Elementor template's display condition is updated.

should clear both the cache and the Used CSS. Currently, it only removes the cache.

If my understanding is correct, the button's text should be replaced:

with "Clear Cache and Used CSS".

At the moment, when clicking the button the notice is displayed again when visiting/refreshing an admin page, i.e. it's not automatically dismissed.

If the notice is dismissed, and the display condition or triggers are changed the notice isn't displayed again for the same user the way it should according to point 6 of the AC.

Lastly, the wpr_elementor_need_purge transient, if there, should be deleted when uninstalling WP Rocket: https://github.com/wp-media/wp-rocket/blob/3a27e9269dd875e00d9d624f51860af2b459130c/inc/Engine/WPRocketUninstall.php#L47-L77

Fixes

Pending @piotrbak 's confirmation, a sum of the changes that are needed:

• [ ] The button in the notice should clear both the cache and the Used CSS - ideally, this should not clear the whole cache/used CSS but only that of the affected posts.
• [ ] The button's wording should include the used CSS clearing part.
• [ ] After clicking it, the notice shouldn't be displayed in admin.
• [ ] If the display condition or triggers for a template are changed the notice should be displayed again.
• [ ] The wpr_elementor_need_purge transient should be deleted when uninstalling WP Rocket.

[piotrbak]

@vmanthos The button is supposed to say Clear Used CSS

ideally, this should not clear the whole cache/used CSS but only that of the affected posts. This is a case when a module (like popup, footer or header, basically, the ones using _elementor_data) is added to specific post: https://github.com/wp-media/wp-rocket/blob/3a27e9269dd875e00d9d624f51860af2b459130c/inc/ThirdParty/Plugins/PageBuilder/Elementor.php#L244

[vmanthos]

@jeawhanlee The used CSS still isn't cleared when clicking the "Clear Used CSS" button.

To reproduce:

1. Edit a popup template and set the "Display Conditions" to "Entire Site".
2. Visit the admin area => The notice will be displayed.
3. Click on the "Clear Used CSS" button and check the wpr_rocket_used_css table => the used CSS won't be removed.

Also, @wp-media/productrocket, if the Remove Unused CSS feature is not enabled and the display condition changes we won't clear the cache and we won't display a notice similar to the one when Remove Unused CSS is enabled. If for example, we set a popup to be displayed site-wide, that won't be displayed until the cache is cleared manually or by an automatic trigger.

How should this be handled?

[DahmaniAdame]

if the Remove Unused CSS feature is not enabled and the display condition changes we won't clear the cache and we won't display a notice similar to the one when Remove Unused CSS is enabled.

We should display a notice to prompt the user to clear the cache since it will cause an inconsistency if it isn't.

[CrochetFeve0251]

if the Remove Unused CSS feature is not enabled and the display condition changes we won't clear the cache and we won't display a notice similar to the one when Remove Unused CSS is enabled.

We should display a notice to prompt the user to clear the cache since it will cause an inconsistency if it isn't.

@DahmaniAdame what should be the message for the new notice?

[DahmaniAdame]

@CrochetFeve0251

Your Elementor template was updated. Clear the cache if the display conditions were changed.

[vmanthos]

@CrochetFeve0251 I can see the message displayed when an Elementor's template is changed while Remove Unused CSS is disabled.

Clicking the "Clear Cache" CTA will clear the whole cache even if the specific template was used for a subset of the site's pages, e.g., only posts or the front page.

@piotrbak I believe this is not the intended behavior, is it?

I'd expect:

1. We'd check the "Display Conditions" for the specific template to
2. Gather the URLs where the template is used and
3. Clear only the affected URLs cache.

By the way, the method you mentioned here doesn't seem to be used anywhere in the code. :thinking:

[MathieuLamiot]

Hello @CrochetFeve0251, @vmanthos, @piotrbak, @DahmaniAdame 👋 This PR has been going on for too long now and maybe it is just me, but I am unable to understand its scope anymore. I am afraid we are chasing a perfect and optimized compatibility with Elementor and hence, we never move forward.

My understanding was that up until know, we did not display the arning message at the right time. It sounds reasonable to fix this not to wrong the user. I understand this part is OK now? @vmanthos, do you confirm? If yes, then can we consider the current version has no regression and close it? If no, what are the issues that make the PR worse than the current release? We would have to uniquely solve those.

If there are follow-up improvements for the compatibility with Elementor, we should have dedicated issues with a well defined scope each. But this one is getting out of hand, so let's close it as soon as possible.

Thanks

[piotrbak]

@MathieuLamiot The scope is pretty straightforward: https://github.com/wp-media/wp-rocket/issues/5848#issuecomment-1658184711

However, the number of going backs to QA is too high, and I don't think we can afford this now. Do you agree to block the issue and later on break it into two parts when we have time? The notice and auto Used CSS clearing is kind of useful and important here.

[MathieuLamiot]

Thanks @piotrbak 🙏 This message is indeed pretty clear but probably lost (at least for me) in all the discussions.

We can block it if there is nothing usable quickly in this PR. Otherwise, we might be able to ship it as is as a first step? Then I agree that we will need to split the remaining stuff in small issues to get it done.

[vmanthos]

It sounds reasonable to fix this not to wrong the user. I understand this part is OK now?

Notifications are displayed correctly @MathieuLamiot.

Currently, we'll be clearing the whole cache when the user clicks the CTA button even if that's not necessary. In the future, we can improve this and make it more fine-grained.