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

Fixes #1685 Auto-active SSL cache for new installations #1734

Merged
merged 4 commits into from May 28, 2019

Conversation

Projects
None yet
3 participants
@Tabrisrp
Copy link
Contributor

commented May 17, 2019

Also fixes #1193

@Tabrisrp Tabrisrp added this to the 3.3.4 milestone May 17, 2019

@Tabrisrp Tabrisrp requested review from Screenfeed and arunbasillal May 17, 2019

@Tabrisrp Tabrisrp self-assigned this May 17, 2019

@arunbasillal

This comment has been minimized.

Copy link
Contributor

commented May 20, 2019

Looks good to me. 👍

Wondering if this will have any influence on a website where WP Rocket fails to detect HTTPS -

// Checked the SSL option if the whole website is on SSL.
if ( rocket_is_ssl_website() ) {
$newvalue['cache_ssl'] = 1;
}

@Tabrisrp

This comment has been minimized.

Copy link
Contributor Author

commented May 21, 2019

It will behave as before for that so I don't think we need to worry about it

@arunbasillal

This comment has been minimized.

Copy link
Contributor

commented May 21, 2019

Thanks. So shall I merge this PR? Not sure if it's you or me.

@Screenfeed

This comment has been minimized.

Copy link
Contributor

commented May 28, 2019

Hi.
I don't understand this one:

  • If I read the title, "Auto-active SSL cache for new installations", this PR should stop at upgrader.php and not remove the checkbox.
  • If I read the issue ticket, the checkbox should indeed be removed, but the option itself and the whole system too (htaccess writes conditions, etc).
  • If somehow we keep it, hidden, filterable: the option value should be removed, and a hook should filter a hard-coded value. Moreover, if I disabled it on my site, then upgrade, I can't enable it back anymore.
    I feel like we're in the 3rd case, but we don't want to force this change to users who disabled the option, am I right?
@Screenfeed

This comment has been minimized.

Copy link
Contributor

commented May 28, 2019

After some explanations from @arunbasillal I understand more the story.
We still found few quirks though. The option would be in a "zombie state" as I call it: it's dead, but still alive somehow, with ways to change its value.

  • Since the checkbox is missing, submitting the settings page will delete the option. If I remember correctly, there is something for "hidden options" somewhere.
  • The function rocket_update_ssl_option_after_save_home_url() may set the option back to 0. A possible solution:
function rocket_update_ssl_option_after_save_home_url( $old_value, $value ) {
        if ( $old_value === $value || get_rocket_option( 'cache_ssl' ) ) {
            return;
        }

The general idea is to prevent the option to be set from 1 to 0, unless the dedicated filter is used.
Looking in the plugin for rocket_is_ssl_website() tests may spot other cases.

@Screenfeed Screenfeed merged commit b349339 into develop May 28, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Screenfeed Screenfeed deleted the fix/1685-ssl-cache branch May 28, 2019

@Tabrisrp Tabrisrp referenced this pull request Jun 4, 2019

Merged

3.3.4 #1754

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.