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

raamdev commented 8 years ago

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 commented 8 years ago

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?

jaswrks commented 8 years ago

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 commented 8 years ago

@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 commented 8 years ago

@jaswsinc Ping. ↑

jaswrks commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

@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 commented 8 years ago

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 commented 8 years ago

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

wpsharks / comet-cache

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