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

CDN rewriting doesn't work when WP_HOME/WP_SITE_URL scheme is different than the site's URLs #2520

Closed
4 tasks done
vmanthos opened this issue Apr 6, 2020 · 4 comments · Fixed by #2589
Closed
4 tasks done
Assignees
Labels
effort: [S] 1-2 days of estimated development time module: CDN priority: high Issues which should be resolved as quickly as possible type: bug Indicates an unexpected problem or unintended behavior
Milestone

Comments

@vmanthos
Copy link
Contributor

vmanthos commented Apr 6, 2020

When the WP_HOME is using http but the site's URLs are using https, e.g. there is an htaccess redirect from http to https CDN rewriting doesn't take place.

This is because of the str_replace() in the rewrite_url method of the CDN class:

return str_replace( $home_url, rocket_add_url_protocol( $cdn_url ), $url );

Example:
http://sandbox.onlinephpfunctions.com/code/1521df9f18dd8b11edf02e77f9994656078f6539

To Reproduce

Steps to reproduce the behavior:

  1. Make sure the WP_HOME and WP_SITE_URL are using http.
  2. Enable http to https redirection in the .htaccess file.
  3. Enable the CDN option.
  4. Check the website. Static assets will not be rewritten to point to the CDN.

Expected behavior
Static assets should be pointing to the CDN.

Additional context

Using the rocket_is_ssl_website() function before the string_replace() resolves the issue:

if( rocket_is_ssl_website() ) {
    $home_parts['scheme'] = 'https';
}

Related ticket:
https://secure.helpscout.net/conversation/1116358022/153068/

Backlog Grooming (for WP Media dev team use only)

  • Reproduce the problem
  • Identify the root cause
  • Scope a solution
  • Estimate the effort
@GeekPress GeekPress added module: CDN needs: grooming priority: high Issues which should be resolved as quickly as possible type: bug Indicates an unexpected problem or unintended behavior labels Apr 6, 2020
@remyperona
Copy link
Contributor

I think this is a duplicate of #2148

@GeekPress
Copy link
Contributor

GeekPress commented Apr 6, 2020

It seems, thanks for the catch @Tabrisrp . I closed #2148 as this contains more details of the problem.

@ScottTravisHartley
Copy link
Contributor

I want to comment I noticed this issue as well working on a client site today their previous dev set it up as the issue describes I updated the home and site URLs to https and the rewrite works again.

@remyperona
Copy link
Contributor

Reproduce the issue
Issue reproduced and reported in multiple occasions

Identify the root cause
As noted here and in the duplicate issue, it's happening when there is a difference between the URL set for home/site URL, and the real URL used, when the scheme is not the same.

The difference in scheme is preventing the str_replace from doing its job correctly.

Scope a solution
str_replace can accept an array for the first argument $needle, so we can use an array under the following format to perform the replacement in all cases:
[ 'http://example.org', 'https://example.org' ]

Effort
Effort [S]

@remyperona remyperona added effort: [S] 1-2 days of estimated development time and removed needs: grooming labels Apr 6, 2020
@GeekPress GeekPress added this to the 3.5.5 milestone Apr 23, 2020
@remyperona remyperona self-assigned this Apr 28, 2020
remyperona added a commit that referenced this issue May 5, 2020
…N URL (PR #2589)

* move and rename files to new architecture
* use home_url() instead of get_option( 'home' )
* add service provider for the CDN module
* improve tests code

Co-authored-by: Tonya Mork <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort: [S] 1-2 days of estimated development time module: CDN priority: high Issues which should be resolved as quickly as possible type: bug Indicates an unexpected problem or unintended behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants