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

Elementor templates clearing cache in full #5848

Open
6 tasks done
DahmaniAdame opened this issue Mar 31, 2023 · 25 comments · May be fixed by #5956
Open
6 tasks done

Elementor templates clearing cache in full #5848

DahmaniAdame opened this issue Mar 31, 2023 · 25 comments · May be fixed by #5956
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 needs: copywriting 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

Comments

@DahmaniAdame
Copy link
Contributor

DahmaniAdame commented Mar 31, 2023

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

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

Describe the bug

In that specific case, it's the popup module. But the same logic should apply to all Elementor's templates.

Templates don't have a permalink structure.

Links look like:

http://domain.ext/?elementor_library=free-delivery-information-popup-1

When these popups are updated and rocket_clean_post is triggered, we extract the path to the root cache folder.

Array
(
    [host] => domain.ext
    [path] => /
    [scheme] => http
    [query] => elementor_library=some-popup-title
    [fragment] => 
)

And we end up purging /public_html/wp-content/cache/wp-rocket/domain.ext/

$entry = $dir . $parsed_url['path'];
// Skip if the dir/file does not exist.
if ( ! $filesystem->exists( $entry ) ) {
continue;
}
if ( $filesystem->is_dir( $entry ) ) {
rocket_rrmdir( $entry, [], $filesystem );
} else {
$filesystem->delete( $entry );
}

rocket_clean_files should be safeguarded against processing that kind of URLs.

We could possibly add an additional check here:

if ( ! empty( $parsed_url['host'] ) ) {

To see if $parsed_url['path'] !== '/'.

In addition to that, ideally, we shouldn't pick up the post permalink when dealing with Elementor's templates.

Instead, we should restrict cache purging to posts where the template, in this example, a popup, is being used.

Assuming that the template post id is 12345

The element will be referenced as part of the _elementor_data custom field when being used on a page.

Inside these data, an entry like the following will be found. Here is an example for popups:

[elementor-tag id=\"42cdb83\" name=\"popup\" settings=\"%7B%22popup%22%3A%2212345%22%7D\"]"}}]

We can have a SQL query with the template ID to check the wp_postmeta table to collect all related posts, and purge them from cache.

We could maybe have a new function to replace rocket_clean_post as the hook to process Elementor templates cache clearing to not over complicate rocket_clean_post logic?

To Reproduce
Steps to reproduce the behavior:

  1. Use Elementor's popups
  2. Buildup cache
  3. Update the popup post
  4. See error

Expected behavior
We shouldn't clear the cache in full in that context.

Screenshots
N/A

Additional context
Ticket - https://secure.helpscout.net/conversation/2179456687/407285/
Issue temporarily fixable using the following helper plugin - https://drive.google.com/file/d/1DAmftPxPWgld3MEyXPWMhmukOqwzsi98/view?usp=sharing

Backlog Grooming (for WP Media dev team use only)

  • Reproduce the problem
  • Identify the root cause
  • Scope a solution
  • Estimate the effort
@piotrbak piotrbak 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 priority: medium Issues which are important, but no one will go out of business. labels Apr 12, 2023
@piotrbak
Copy link
Contributor

piotrbak commented May 6, 2023

Acceptance Criteria

  • rocket_clean_post should be guarded against clearing the whole cache (possible relation Security Plugin(s) renders wp-rocket useless #5887)
  • Clear the cache of posts that contain updated Elementor module
  • No regressions in rocket_clean_post when updating the regular posts

@jeawhanlee jeawhanlee added GROOMING IN PROGRESS Use this label when the issue is currently being groomed. effort: [S] 1-2 days of estimated development time and removed needs: grooming GROOMING IN PROGRESS Use this label when the issue is currently being groomed. labels May 11, 2023
@jeawhanlee
Copy link
Contributor

jeawhanlee commented May 12, 2023

Reproduce the problem ✅

I was able to reproduce the problem.

Identify the root cause ✅

Just like @DahmaniAdame Stated

Scope a solution ✅

To stop full cache when updating a template in elementor, we can do a check in

like @DahmaniAdame stated

if ( '/' === $parsed_url['path'] || '' === $parsed_url['path'] ) {
  continue;
}

To remove associated posts when elementor modules are updated are 2 ways

  • For Templates that are used directly in posts, elementor saves meta_key relative to that post like @DahmaniAdame described.
    We will add a new event - elementor/editor/after_save in the elementor 3rd party class

    public static function get_subscribed_events() {
    and create a callback method:
    clear_related_post_cache( $post_id )
    In this method we will then use the $post_id to query the postmeta table to return rows with $post_id in meta_value column and clear cache of posts with rocket_clean_post

  • For Templates with conditions, elementor saves this as _elementor_conditions in the postmeta table with various value(not all are included):

    • include/general - entire site
    • include/singular - pretty much like entire site.
    • include/singular/front_page - only front page/homepage
    • include/singular/post - all posts
    • include/singular/page - all pages
    • include/singular/post/9 - specific post with post_id as 9
    • include/singular/page/2 - specific page with post_id as 2
    • include/singular/not_found404 - 404 page

We can do checks against this values and clear as expected.
Add tests too.

Estimate the effort ✅

[S]

@engahmeds3ed What do you think?

@piotrbak
Copy link
Contributor

@jeawhanlee @engahmeds3ed The effort here is [M], not [S], right?

@jeawhanlee jeawhanlee self-assigned this May 24, 2023
@jeawhanlee jeawhanlee linked a pull request May 31, 2023 that will close this issue
9 tasks
@DahmaniAdame
Copy link
Contributor Author

@piotrbak can we add clearing the Used CSS as a requirement for this one? Or should I open a separate issue for it?

@piotrbak
Copy link
Contributor

@DahmaniAdame Just to be sure, it's about clearing the Used CSS of related posts, right?

@DahmaniAdame
Copy link
Contributor Author

@piotrbak yes, exactly. If the template is edited, the style will change. What will happen is that we will be shipping the wrong style on related posts if we just clear cache. Both cache and Used CSS should be cleared.

@valmirselmani
Copy link

@piotrbak Can you also provide the ability to disable cache clearing when templates are edited? In my case, I have many templates that affect only one page. The site has over 1000 pages, and every time the editor makes changes to the text, the entire cache is cleared.

@valmirselmani
Copy link

@piotrbak It's the same issue. When we edit Elementor templates, the cache is cleared in full. I just like to have the possibility to disable clearing the cache in full with a filter when templates are edited.

@piotrbak
Copy link
Contributor

piotrbak commented Jul 17, 2023

@DahmaniAdame I think this should be limited to specific templates only. As per @wp-media/qa and I also confirmed this on my website, if you edit the regular template using free Elementor, the changes are not applied to the pages that contain this template. Steps to reproduce :

  1. Create a template, Templates > Add new section template
  2. Add it to the page
  3. Edit the template
  4. Check the page, the template is not updated

The templates that are auto-updating are for sure:

  1. Popups
  2. Footers
  3. Headers

@piotrbak piotrbak added waiting for feedback status: blocked Issue or PR is blocked by external factor. labels Jul 17, 2023
@piotrbak
Copy link
Contributor

@valmirselmani Yes, I can see that. For now you could modify the following helper plugin to achieve what you want:
https://github.com/wp-media/wp-rocket-helpers/blob/master/cache/wp-rocket-no-cache-auto-purge/wp-rocket-no-cache-auto-purge.php

@DahmaniAdame
Copy link
Contributor Author

DahmaniAdame commented Jul 18, 2023

@piotrbak

the template is not updated

The regular templates are like partials to be added to pages. They are not dynamic, as far as I know. Updating a template won't update the page where it was inserted.

It's not expected that the page's template content will change if the template is update. And we are not expected to clear the page's cache if the template is updated.

I think this should be limited to specific templates only

I believe it already is. It's supposed to target only templates adding entries to the post's _elementor_data.

@valmirselmani current issue is what this ticket is about. WP Rocket converts the template's path to become / and causes cache to be cleared in full.

This is what this part will fix -

if ( '/' === $parsed_url['path'] ) {
continue;
}

@valmirselmani for now, using the helper plugin to disable automatic cache clearing as advised by Piotr should help.

@piotrbak
Copy link
Contributor

@DahmaniAdame The issue initially was about the all templates, was surprised while checking the QA report + doing some research 😁

Sending it back to QA then. Do you have a list of templates that are triggering the cache clearing or it should be explored?

@DahmaniAdame
Copy link
Contributor Author

DahmaniAdame commented Jul 18, 2023

@piotrbak the first part, where all cache is cleared, is about all templates!

So, what needs to be checked by @wp-media/qa is if cache is still fully clear if any template updated.

It's the second part where specific posts are cleared related to dynamic template, let's call them that, that is specific to the part where we look for _elementor_data.

For this one, what needs to be checked is if all posts with the specific popup for example added are the only ones being cleared.

Do you have a list of templates that are triggering the cache clearing or it should be explored?

Unfortunately, no.

I tested with Popups initially. But Header/Footer should have the same logic.

Add-ons with popups or handling at least these 3 types are likely piggybacking on that logic as well.

Basically, whatever adds a template element with an entry on the post's _elementor_data would qualify for the fix.

@piotrbak piotrbak removed waiting for feedback status: blocked Issue or PR is blocked by external factor. labels Jul 18, 2023
@valmirselmani
Copy link

@DahmaniAdame
Copy link
Contributor Author

@valmirselmani can you open a ticket for someone to have a closer look?

@piotrbak
Copy link
Contributor

I'm blocking this one for now, it needs a product discussion a bit, will move it back to grooming as soon as we have all the answers.

@piotrbak piotrbak added the status: blocked Issue or PR is blocked by external factor. label Jul 20, 2023
@piotrbak
Copy link
Contributor

@DahmaniAdame This issue became more complicated than we thought it'll be. To avoid releasing important regressions, I'd suggest adding to AC:

  1. Clear the whole cache when display conditions are changed within the template
  2. If the template is used site-wide and it's updated, we should clear the whole cache (but not Used CSS, as the template could have been updated jsut by changing the text)

Also, we should preserve the current functionality already introduced in this PR:

  1. rocket_clean_post should be guarded against clearing the whole cache
  2. Clear the cache and used CSS of posts that contain updated Elementor module
  3. No regressions in rocket_clean_post when updating the regular posts

If we introduce the above, there'll be regression related to not clearing cache of the Templates with Conditions, as per @jeawhanlee:
- include/singular - pretty much like entire site.
- include/singular/front_page - only front page/homepage
- include/singular/post - all posts
- include/singular/page - all pages
- include/singular/post/9 - specific post with post_id as 9
- include/singular/page/2 - specific page with post_id as 2
- include/singular/not_found404 - 404 page

Handling all those scenarios with cache clearing will be complex and time-consuming, on the other hand, right now when saving the template we're clearing everything.

@DahmaniAdame
Copy link
Contributor Author

@piotrbak let's go with that.

If the template is used site-wide and it's updated, we should clear the whole cache (but not Used CSS, as the template could have been updated jsut by changing the text)

Do we have a way to know if it's just text or get Elementor involved to provide a way to do so?

If yes, we can use that to clear the Used CSS selectively if not just text is changed.

If not, we should go with clearing the Used CSS to avoid risks of displaying a broken page.

@piotrbak
Copy link
Contributor

@DahmaniAdame Are we okay with this part?

If we introduce the above, there'll be regression related to not clearing cache of the Templates with Conditions, https://github.com/wp-media/wp-rocket/issues/5848#issuecomment-1545701597

We could either:

  1. Automatically clear cache + Used CSS sitewide
  2. Display a notice telling customer about the update, so he's aware of the change and let him decide what to do
  3. Try to find a way to make a connection between conditions and URLs in cache, which will be time consuming and might break in the future if Elementor changes anything in the logic

@piotrbak piotrbak removed the effort: [S] 1-2 days of estimated development time label Jul 25, 2023
@DahmaniAdame
Copy link
Contributor Author

@piotrbak

Are we okay with this part?

Yes. That's acceptable.

Can we involve Elementor and see if they can offer a hook for caching plugins to handle that part?

Otherwise, option 2 looks like a good middle ground. 1 might cause an unnecessary cache/RUCSS cycle. And 3 will be high maintenance.

@piotrbak
Copy link
Contributor

piotrbak commented Jul 31, 2023

To follow-up on this one, with new acceptance criteria. We'll preserve the current functionality of this PR:

  1. rocket_clean_post should be guarded against clearing the whole cache ✔️
  2. Clear the cache and used CSS (if it's enabled) of posts that contain updated Elementor module ✔️
  3. No regressions in rocket_clean_post when updating the regular posts ✔️

And for the second scenario, with the condition templates:

  1. Whenever Elementor Condition Template is updated and Used CSS is enabled, we'll display a dismissible warning on all admin pages ✔️
  2. Whenever Elementor Condition Template is updated and Used CSS is disabled, notice will not be displayed. ✔️
  3. Wording will be as following: WP Rocket: Your Elementor template was updated. Clear the Used CSS if the layout, design or CSS styles were changed. ✔️
  4. After clicking on Clear Used CSS, we'll purge the Used CSS and Cache ✔️
  5. After clicking on Dismiss, the message will disappear ✔️
  6. If the Elementor Condition Template will be updated in the future, the message should be displayed once again ✔️

@piotrbak piotrbak added needs: grooming and removed status: blocked Issue or PR is blocked by external factor. labels Jul 31, 2023
@CrochetFeve0251
Copy link
Contributor

CrochetFeve0251 commented Aug 7, 2023

Reproduce the problem

I saw @jeawhanlee was able to reproduce the problem on the first grooming and explained the root cause.

Scope a solution

The solution we gonna have here will have to reuse as much as possible.
For guarding against clearing the whole cache we will reused that logic.

'rocket_pre_clean_post' => [ 'exclude_post_type_cache_clearing', 10, 2 ],
/**
	 * Exclude elementor library post type from cache clearing.
	 *
	 * @param mixed $clear_post A preemptive return value. Default null.
	 * @param Post  $post Post Object.
	 * @return boolean
	 */
	public function exclude_post_type_cache_clearing( $clear_post, $post ) {
		if ( 'elementor_library' === $post->post_type ) {
			return true;
		}

		return $clear_post;
	}

Then concerning the modal we will have to first handle the logic that will detect a change on the elementor template.
For that I propose to create a second callback on update_elementor_library_metadata that will setup a transient.

function setup_transient($check, $object_id, $meta_key, $meta_value, $prev_value) {
   if('_elementor_conditions' !== $meta_key || $meta_value === $prev_value || ! $this->options->get('', false)) {
     return;
   }
   set_transient('wpr_elementor_need_purge', true);
   $boxes = get_user_meta( get_current_user_id(), 'rocket_boxes', true );
   unset($boxes['maybe_clear_cache_change_notice'']);
}

Once we done that then we have to handle the notice by adding the following callback on admin_notices:

public function maybe_clear_cache_change_notice() {

		$boxes = get_user_meta( get_current_user_id(), 'rocket_boxes', true );

		if ( in_array( __FUNCTION__, (array) $boxes, true ) ) {
			return;
		}

		if ( ! current_user_can( 'rocket_manage_options' ) ) {
			return;
		}

		$notice = get_transient( 'wpr_elementor_need_purge' );

		if ( ! $notice ) {
			return;
		}

		$args = [
			'status'         => 'warning',
			'dismissible'    => '',
			'dismiss_button'         => __FUNCTION__,
			'message'        => sprintf(
			// translators: %1$s = <strong>, %2$s = </strong>, %3$s = <a>, %4$s = </a>.
				__( '%1$sWP Rocket:%2$s Your Elementor template was updated. Clear the Used CSS if the layout, design or CSS styles were changed.', 'rocket' ),
				'<strong>',
				'</strong>',
				'</a>'
			),
			'action'         => 'clear_cache',
		];

		rocket_notice_html( $args );
	}

Also we need to handle the clearing from related posts when an Elementor module is cleared. For that we need to hook the following callback to elementor/editor/after_save:

public function clear_related_post_cache( int $post_id ) {
		global $wpdb;

		$template_id = '%' . $wpdb->esc_like( $post_id ) . '%';

		// phpcs:ignore WordPress.DB.PreparedSQL.NotPrepared, WordPress.DB.DirectDatabaseQuery.DirectQuery, WordPress.DB.DirectDatabaseQuery.NoCaching
		$results = $wpdb->get_results(
				$wpdb->prepare(
				"SELECT post_id FROM `$wpdb->postmeta` WHERE `meta_key` = %s AND `meta_value` LIKE %s",
				[ '_elementor_data', $template_id ]
				)
			);

		if ( ! $results ) {
			return;
		}

		foreach ( $results as $result ) {
			if ( 'publish' === get_post_status( $result->post_id ) ) {
				rocket_clean_post( $result->post_id );

				if ( $this->options->get( 'remove_unused_css', 0 ) ) {
					$url = get_permalink( $result->post_id );
					$this->used_css->delete_used_css( $url );
				}
			}
		}
	}

Estimate the effort

Effort S

@CrochetFeve0251 CrochetFeve0251 added effort: [S] 1-2 days of estimated development time and removed needs: grooming labels Aug 7, 2023
@CrochetFeve0251 CrochetFeve0251 self-assigned this Sep 6, 2023
@CrochetFeve0251 CrochetFeve0251 added the status: blocked Issue or PR is blocked by external factor. label Sep 8, 2023
@CrochetFeve0251
Copy link
Contributor

@piotrbak I would need a elementor pro license to test the implmentation the old one look like expired

@CrochetFeve0251 CrochetFeve0251 removed the status: blocked Issue or PR is blocked by external factor. label Sep 8, 2023
@CrochetFeve0251
Copy link
Contributor

For the value check with the previous values. In the current state it will be always different as the previous value always return ''.
However Elementor handle that case from the UI and prevent the user from saving.

@camilamadronero
Copy link

Related case here: https://secure.helpscout.net/conversation/2481477875/469222?folderId=2675957
Slack thread: https://wp-media.slack.com/archives/C43T1AYMQ/p1705678445916809
URL passed to partial cache purge: https://staging1.ksmedizintechnik.de/?cms_block=home-kw51
Fixed with a helper plugin by Alfonso.

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 needs: copywriting 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.

6 participants