wp-media / wp-rocket

Performance optimization plugin for WordPress
https://wp-rocket.me
GNU General Public License v2.0
701 stars 218 forks source link

RocketCDN CNAME change #7027

Open piotrbak opened 4 weeks ago

piotrbak commented 4 weeks ago

Is your feature request related to a problem? Please describe. In the upcoming version we'll need to change the RocketCDN CNAMEs automatically

Describe the solution you'd like The Acceptance Criteria is described here: https://www.notion.so/wpmedia/RocketCDN-CNAME-change-d72cd2baa88b49b7b4cb65b348440564?pvs=4

Khadreal commented 3 weeks ago

Scope a solution ✅

Add new method to handle the notice for RocketCDN here and the newly created method to the admin_notices filter here.

The method created above should only be displayed based on this conditions here

Then in this file add a new method

public function update_cdn_name( $new_version, $old_version ): void {
        if ( version_compare( $old_version, '3.17.2', '>' ) ) {
            return;
        }
                delete_transient('rocketcdn_status');

        $this->api_client->get_subscription_data();
    }

Add the APiClient as constructor to Subscriber

Finally, add the method created as a callback to wp_rocket_upgrade

Add test, update existing ones.

Estimation

[S]

MathieuLamiot commented 3 weeks ago

As the approach is based on the RocketCDN API, the plugin release with this change must happen after this is released: https://github.com/wp-media/rocket-cdn/issues/546 Depending on the delay on the service team side, we might want to take this out of 3.17.2

MathieuLamiot commented 2 weeks ago

Removing the 3.17.2 milestone as the RocketCDN API won't be ready for 3.17.2.

wordpressfan commented 2 weeks ago

We'll display a (success/warning/info) dismissible notice in the WP Rocket plugin settings page

This point is still not handled by this grooming, correct? @Khadreal

Khadreal commented 2 weeks ago

It's the notice in WPR is dismissible by default, so I don't think it's needed to cover that again.

wordpressfan commented 2 weeks ago

I'm not talking about the dismiss, I'm talking about the notice itself, I believe we need to show it in WPR settings along with rocketCDN settings page. cc @piotrbak

piotrbak commented 2 weeks ago

@wordpressfan Yes, the notice should be there.

@DahmaniAdame I think we should display a blue, notification notice. WDYT?

DahmaniAdame commented 2 weeks ago

@piotrbak I agree. It's informational and non-critical. Blue will do.

We can use orange if we need a second message later in the process if we notice that the former CNAME is still heavily used.

MathieuLamiot commented 2 weeks ago

Grooming to complete with the notice, as pointed out above, before moving it to Grooming to Review.

remyperona commented 2 weeks ago

Everything related to RocketCDN should be handled in RocketCDN classes. The update of the CDN cname can be done in the DataManagerSubscriber class, which already has the API client as a dependency.

We also need to take into account saving the old CDN URL to be able to display it in the notice.

Moving it back to grooming to do.

wordpressfan commented 3 days ago

Grooming

Will take the grooming mentioned above by @Khadreal with some small changes.

Proposed Solution

In this subscriber:

https://github.com/wp-media/wp-rocket/blob/73488699424f726bef28dbf2c934340efcab8e47/inc/Engine/CDN/RocketCDN/DataManagerSubscriber.php#L50

Add a new callback to that array update_cdn_name, this method will be triggered with update action wp_rocket_upgrade, and will save the old cdn url then force reloading subscription data again as below:

public function update_cdn_name( $new_version, $old_version ): void {
    if ( version_compare( $old_version, '3.17.2', '>' ) ) {
        return;
    }

    $old_subscription_data = $this->api_client->get_subscription_data();

    if ( ! $old_subscription_data['is_active'] || empty( $old_subscription_data['cdn_url'] ) ) {
        return;
    }

    update_option( 'rocketcdn_old_url', $old_subscription_data['cdn_url'] );

    delete_transient('rocketcdn_status');

    $this->api_client->get_subscription_data();
}

For showing the notice, We will create a new view file here:

https://github.com/wp-media/wp-rocket/blob/d70151f977efdf2e57a5c6c863ffd8f31a70a905/inc/Engine/CDN/RocketCDN/views

with the notice HTML contents, then we will create a new method here:

https://github.com/wp-media/wp-rocket/blob/ce7b6014eba3777825b2a4cacf942f6b0e7300be/inc/Engine/CDN/RocketCDN/NoticesSubscriber.php#L13

To get the old cdn url from the option rocketcdn_old_url mentioned above and the new cdn url, then will call the generate method to show the view file that we created above.

We may need to remove the option rocketcdn_old_url with notice dismiss, also with plugin removal.

Then adjust tests.

Effort

[S]

Khadreal commented 3 days ago

LGTM