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

Upgrading to version >= 160706 resets Client-Side Cache option #807

Closed
raamdev opened this Issue Jul 13, 2016 · 9 comments

Comments

Projects
None yet
2 participants
@raamdev
Member

raamdev commented Jul 13, 2016

When upgrading to Comet Cache v160706 or v160709, the Client-Side Cache option is reset to the default of "No" (disabled).

The option name for Client-Side Cache was renamed from allow_browser_cache to allow_client_side_cache in v160706, but this routine was supposed to copy the existing value to the new option name during an upgrade to preserve the current setting. For some reason, that's not working the way it should.

I confirmed that the Exclusion Patterns for Client-Side Caching do not get reset—it's only the option that turns on/off this feature.


Originally reported on the Community Forum:
https://wordpress.org/support/topic/update-changes-client-side-cache-to-no

@raamdev raamdev added the bug label Jul 13, 2016

@raamdev raamdev added this to the Next Release milestone Jul 13, 2016

@raamdev

This comment has been minimized.

Member

raamdev commented Jul 13, 2016

It looks like the problem here is that the options are updated with the default options of the new version before the version upgrade routines are run, in which case it's too late to check the value of the old Client-Side Cache option.

That being the case, it would appear that other version upgrade routines that attempt to fetch the existing options and then do something with them are also broken.

It looks like we need to store and pass all of the options before we update the options using the new version:

        $prev_options = $this->options;
        if (version_compare($prev_options['version'], VERSION, '>=')) {
            return; // Nothing to do; up-to-date.
        }
        $this->updateOptions(['version' => VERSION]);

        new Classes\VsUpgrades($prev_options);

(The constructor in the VSUpgrades class, along with any upgrade handlers in the class, would need to be updated appropriately as well.)

@jaswsinc Do you concur?

@raamdev raamdev self-assigned this Jul 13, 2016

@jaswrks

This comment has been minimized.

Member

jaswrks commented Jul 14, 2016

Ah, great catch. Looks like a bug to me also.

  • I'd suggest we add a new parameter to this function with the name $intersect and have it default to a true value.
  • If $intersect is passed as false, this line should not run, which would leave all of the existing option keys alone.
  • With that in place, you can then update this call to updateOptions() and pass the $intersect argument as false.

It might be a good idea for $intersect = false to impact this in some way also.

@raamdev

This comment has been minimized.

Member

raamdev commented Jul 14, 2016

@jaswsinc We have 21 calls to updateOptions() throughout the codebase that should also be updated to reflect the new $intersect parameter. However, I'm not sure we need a whole new parameter to updateOptions() to fix this issue.

In what other scenario would we use $intersect = false? I cannot think of any.

The problem we need to solve is getting a copy of the previous options over to the VsUpgrades class so that the upgrade handlers have access to any information they need regarding the state of the options in the previous version. The upgrade handlers then update the state of the options in the current version as necessary. Once the upgrade handlers have run, the previous plugin options should be irrelevant.

It seems messy and confusing to allow updateOptions() to preserve options that should (possibly) not exist in the current version.

@raamdev

This comment has been minimized.

Member

raamdev commented Jul 19, 2016

@jaswsinc Ping. ↑

@jaswrks

This comment has been minimized.

Member

jaswrks commented Jul 19, 2016

We have 21 calls to updateOptions() throughout the codebase that should also be updated to reflect the new $intersect parameter.

You shouldn't need to update any of those, because the parameter can be added with a default parameter value of true so that it doesn't change the behavior for any existing calls.

The problem we need to solve is getting a copy of the previous options over to the VsUpgrades class so that the upgrade handlers have access to any information they need

Got it. That's why I consider this line to be problematic. So when you ask what use case there would be for $intersect = false, this is that use case.

By not intersecting keys in this instance, we are able to save the options (i.e., not fundamentally change the duty this routine has), but also, make it smarter, and not discard old option keys, for the moment; leaving that for normal behavior later, once VS upgrades are complete.

It seems messy and confusing to allow updateOptions() to preserve options that should (possibly) not exist in the current version.

That would only be true if the parameter is used incorrectly.

Old option keys do exist when upgrading. However, they are no longer read by the current release. They are only useful to the VS upgrade routines.

So while they exist for a moment in time, for the purpose of reading them in the VS upgrade routines, just as soon as a normal update occurs anything old is discarded automatically. In a typical upgrade this would all take place in a single process.

There's more than one way to do this though, and as you said, we don't need to add a new parameter. It was just one idea. I would consider it more messy to pass the options directly to the VS upgrade constructor though. I think the VS upgrade class should read options w/ standard functions, and also save options using standard functions.

Which brings me to another use case for the $intersect parameter. See this line, where saving options in a VS upgrade that runs in sequence with others, also creates a problem. If $intersect is true (the default behavior), then one VS upgrade routine tosses old option keys ahead of the next one running. In short, all of the VS upgrade routines should pass $intersect as false.

It's only after all VS upgrades are complete, that old keys should be removed.

@jaswrks

This comment has been minimized.

Member

jaswrks commented Jul 19, 2016

It may also help if we add an $intersect parameter to the getOptions() method too.

Currently, VS upgrade routines read options using get_site_option() instead of using the standard methods that we have for reading options in the Comet Cache context. Adding a new $intersect parameter might make it possible for VS upgrade handlers to use getOptions() in Comet Cache instead of get_site_option().

@raamdev

This comment has been minimized.

Member

raamdev commented Jul 19, 2016

@jaswsinc writes...

Which brings me to another use case for the $intersect parameter. See this line, where saving options in a VS upgrade that runs in sequence with others, also creates a problem. If $intersect is true (the default behavior), then one VS upgrade routine tosses old option keys ahead of the next one running. In short, all of the VS upgrade routines should pass $intersect as false.

It's only after all VS upgrades are complete, that old keys should be removed.

Good point. I see now why that approach works better. Thanks for explaining! :-)

It may also help if we add an $intersect parameter to the getOptions() method too.

Agreed.

raamdev added a commit to websharks/comet-cache-pro that referenced this issue Sep 6, 2016

raamdev added a commit to websharks/comet-cache-pro that referenced this issue Sep 6, 2016

raamdev added a commit to websharks/comet-cache-pro that referenced this issue Sep 6, 2016

Add $intersect param to getOptions(), don't intersect on startup
We cannot intersect the options via getOptions() when they're first
loaded in Plugin.php, as that will cause us to lose old options that
may be needed by the VS upgrade routines.

See websharks/comet-cache#807

raamdev added a commit to websharks/comet-cache-pro that referenced this issue Sep 6, 2016

@raamdev

This comment has been minimized.

Member

raamdev commented Sep 7, 2016

Next Release Changelog:

  • Bug Fix: Fixed a bug where upgrading from v160521 would result in the Client-Side Cache option being reset to the default (disabled). If you enabled the Client-Side Cache at some point, now is a good time to double-check that it's still enabled. This bug fix also improves the reliability of all version upgrade routines that Comet Cache runs during upgrades. See Issue #807.

@raamdev raamdev closed this Sep 7, 2016

raamdev added a commit to websharks/comet-cache-pro that referenced this issue Sep 7, 2016

raamdev added a commit that referenced this issue Sep 17, 2016

Phing release of v160917 with the following changes:
- **New Feature** (Lite): The Clear Cache button is now available in the Admin Toolbar for the Lite version of Comet Cache.
- **New Feature** (Pro): Comet Cache Pro is now fully compatible with [WordPress Automatic Background Updates](https://codex.wordpress.org/Configuring_Automatic_Background_Updates#Plugin_.26_Theme_Updates_via_Filter). If you enable automatic background updates for plugins, and you save valid Comet Cache Pro License Credentials in the _Comet Cache Pro → Plugin Options → Update Credentials_ panel, you will automatically receive Pro plugin updates. Props @jaswsinc. See [Issue #289](#289).
- **Bug Fix**: In some scenarios Comet Cache might produce a false-positive "Warning: mkdir(): File exists" message when checking if the cache directory exists. Comet Cache now calls `clearstatcache()` and uses `file_exists()` instead of `is_dir()` to help make this check more robust. See [Issue #786](#786).
- **Bug Fix**: Fixed a bug where the Comet Cache PHP requirements check would fail and produce a fatal error when upgrading from a version of Comet Cache that did not require an extension that is now required by newer releases. This would occur when, for example, the required PHP `mbstring` extension was missing. Props @jaswsinc for finding the bug. See [Issue #817](#817).
- **Bug Fix**: Fixed a bug where upgrading from v160521 would result in the Client-Side Cache option being reset to the default (disabled). If you enabled the Client-Side Cache at some point, now is a good time to double-check that it's still enabled. This bug fix also improves the reliability of all version upgrade routines that Comet Cache runs during upgrades. See [Issue #807](#807).
- **Compatibility / Bug Fix**: The automatic Clear Cache routines that cleared the entire cache automatically whenever _WordPress Dashboard → Settings → General_ was updated, were being too aggressive and not taking into consideration other plugins that might also be using the same `options-general.php` URL. As a result, the entire cache was being unnecessarily cleared when the settings for those other plugins were saved. Props to @futtta from Autoptimize for reporting. See [Issue #825](#825).
- **UI Enhancement:** Adjusted option page font styles for WordPress v4.6 to better match existing style. See [Issue #271](websharks/comet-cache-pro#271).
- **ManageWP Compatibility** (Pro): Comet Cache Pro is now compatible with ManageWP, a service that allows remote management of multiple WordPress sites. Comet Cache Pro Plugin Updates will now appear in the ManageWP dashboard and, assuming you have saved valid license credentials in _Dashboard → Comet Cache Pro → Plugin Options → Update Credentials_, you will be able to upgrade Comet Cache Pro remotely from the ManageWP Dashboard. Props @jaswsinc. See [Issue #465](#465).
- **InfiniteWP Compatibility** (Pro): Comet Cache Pro is now compatible with InfiniteWP, an application that allows you to manage multiple WordPress sites from a single location. Comet Cache Pro Plugin Updates will now appear in the InfiniteWP dashboard and, assuming you have saved valid license credentials in _Dashboard → Comet Cache Pro → Plugin Options → Update Credentials_, you will be able to upgrade Comet Cache Pro remotely from the InfiniteWP Dashboard. See [Issue #394](#394).
- **Rewritten Pro Plugin Updater**: The Comet Cache Pro Plugin Updater has been redesigned to use the built-in WordPress plugin updater system. When a Comet Cache Pro update is available, it now appears in the WordPress Updates section and in the Plugins list, like other WordPress plugins and can be updated normally like other WordPress plugins, as long as you have saved valid Comet Cache Pro license details in the new "Update Credentials" options panel. Props @jaswsinc. See [Issue #272](websharks/comet-cache-pro#272).
- **Code Style**: The `WP_CACHE` line that gets inserted into the `wp-config.php` file to enable caching now follows the [WordPress PHP Code Standards](https://make.wordpress.org/core/handbook/best-practices/coding-standards/php/). Props @szepeviktor. See [Issue #799](#799).
- **Compatibility** (Pro): When the Autoptimize plugin is active, the Comet Cache Pro HTML Compressor panel now shows a friendly notice explaining that both the HTML Compressor and Autoptimize should not be enabled at the same time because they both address the same performance improvements. The rest of Comet Cache works great alongside Autoptimize and whether you use the HTML Compressor or Autoptimize is a matter of preference. Props to @futtta from Autoptimize for the continued collaboration.
@raamdev

This comment has been minimized.

Member

raamdev commented Sep 17, 2016

Comet Cache v160917 has been released and includes changes from this GitHub Issue. See the v160917 announcement for further details.


This issue will now be locked to further updates. If you have something to add related to this GitHub Issue, please open a new GitHub Issue and reference this one (#807).

@websharks websharks locked and limited conversation to collaborators Sep 17, 2016

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.