Elementor templates clearing cache in full

DahmaniAdame commented 1 year ago

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

[x] Made sure you’re on the latest version 3.13
[x] 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/

https://github.com/wp-media/wp-rocket/blob/1f8dfc9660a112a2a937e2fb90e51cad0b9572a5/inc/functions/files.php#L590-L600

rocket_clean_files should be safeguarded against processing that kind of URLs.

We could possibly add an additional check here:

https://github.com/wp-media/wp-rocket/blob/1f8dfc9660a112a2a937e2fb90e51cad0b9572a5/inc/functions/files.php#L588

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:

Use Elementor's popups
Buildup cache
Update the popup post
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)

[x] Reproduce the problem
[x] Identify the root cause
[x] Scope a solution
[x] Estimate the effort

piotrbak commented 1 year ago

Acceptance Criteria

rocket_clean_post should be guarded against clearing the whole cache (possible relation https://github.com/wp-media/wp-rocket/issues/5887)
Clear the cache of posts that contain updated Elementor module
No regressions in rocket_clean_post when updating the regular posts

jeawhanlee commented 1 year ago

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 https://github.com/wp-media/wp-rocket/blob/1f8dfc9660a112a2a937e2fb90e51cad0b9572a5/inc/functions/files.php#L587 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 https://github.com/wp-media/wp-rocket/blob/1f8dfc9660a112a2a937e2fb90e51cad0b9572a5/inc/ThirdParty/Plugins/PageBuilder/Elementor.php#L53 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 commented 1 year ago

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

DahmaniAdame commented 1 year ago

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

piotrbak commented 1 year ago

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

DahmaniAdame commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

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

Create a template, Templates > Add new section template
Add it to the page
Edit the template
Check the page, the template is not updated

The templates that are auto-updating are for sure:

Popups
Footers
Headers

piotrbak commented 1 year ago

@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 commented 1 year ago

@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 - https://github.com/wp-media/wp-rocket/blob/457d4c5cd23c6d6435c17cba4c36adea9c247e36/inc/functions/files.php#L590-L592

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

piotrbak commented 1 year ago

@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 commented 1 year ago

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

valmirselmani commented 1 year ago

@piotrbak @DahmaniAdame I already have this https://github.com/wp-media/wp-rocket-helpers/blob/master/cache/wp-rocket-no-cache-auto-purge/wp-rocket-no-cache-auto-purge.php in place, but it still clears the cache in full.

DahmaniAdame commented 1 year ago

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

piotrbak commented 1 year ago

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 commented 1 year ago

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

Clear the whole cache when display conditions are changed within the template
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:

rocket_clean_post should be guarded against clearing the whole cache
Clear the cache and used CSS of posts that contain updated Elementor module
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 commented 1 year ago

@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 commented 1 year ago

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

Automatically clear cache + Used CSS sitewide
Display a notice telling customer about the update, so he's aware of the change and let him decide what to do
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

DahmaniAdame commented 1 year ago

@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 commented 1 year ago

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

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

And for the second scenario, with the condition templates:

Whenever Elementor Condition Template is updated and Used CSS is enabled, we'll display a dismissible warning on all admin pages :heavy_check_mark:
Whenever Elementor Condition Template is updated and Used CSS is disabled, notice will not be displayed. :heavy_check_mark:
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. :heavy_check_mark:
After clicking on Clear Used CSS, we'll purge the Used CSS and Cache :heavy_check_mark:
After clicking on Dismiss, the message will disappear :heavy_check_mark:
If the Elementor Condition Template will be updated in the future, the message should be displayed once again :heavy_check_mark:

CrochetFeve0251 commented 1 year ago

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 commented 1 year ago

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

CrochetFeve0251 commented 1 year ago

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 commented 9 months ago

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.

wp-media / wp-rocket