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

Closes #5848: Elementor templates clearing cache in full #5956

Open
wants to merge 51 commits into
base: develop
Choose a base branch
from

Conversation

jeawhanlee
Copy link
Contributor

@jeawhanlee jeawhanlee commented May 31, 2023

Description

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

Type of change

  • 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:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • 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 jeawhanlee added type: enhancement Improvements that slightly enhance existing functionality and are fast to implement 3rd party compatibility Issues related to 3rd party compatibility like theme, plugin or hosting module: cache labels May 31, 2023
@jeawhanlee jeawhanlee self-assigned this May 31, 2023
@vmanthos vmanthos self-requested a review May 31, 2023 14:11
@jeawhanlee jeawhanlee marked this pull request as ready for review June 15, 2023 13:45
@jeawhanlee
Copy link
Contributor Author

@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
Copy link
Contributor

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

@remyperona
Copy link
Contributor

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

@piotrbak
Copy link
Contributor

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

@jeawhanlee
Copy link
Contributor Author

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

@piotrbak Yes we can

@jeawhanlee jeawhanlee requested a review from a team July 13, 2023 15:53
inc/common/purge.php Outdated Show resolved Hide resolved
inc/ThirdParty/Plugins/PageBuilder/Elementor.php Outdated Show resolved Hide resolved
@vmanthos
Copy link
Contributor

@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?

@CrochetFeve0251 CrochetFeve0251 requested review from remyperona and a team September 29, 2023 09:13
@DahmaniAdame
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

@CrochetFeve0251

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

@vmanthos
Copy link
Contributor

@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. 🤔

@MathieuLamiot
Copy link
Contributor

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
Copy link
Contributor

@MathieuLamiot The scope is pretty straightforward:
#5848 (comment)

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
Copy link
Contributor

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
Copy link
Contributor

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.

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 module: cache type: enhancement Improvements that slightly enhance existing functionality and are fast to implement waiting for feedback
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Elementor templates clearing cache in full
7 participants