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

CSS/JS minification/combination not working on Kinsta when the site's URL contains a path #3111

Open
4 tasks
vmanthos opened this issue Sep 17, 2020 · 6 comments
Labels
3rd party compatibility Issues related to 3rd party compatibility like theme, plugin or hosting module: combine CSS module: combine JS module: minify CSS module: minify JS priority: medium Issues which are important, but no one will go out of business. severity: major Feature is not working as expected and no work around available type: bug Indicates an unexpected problem or unintended behavior

Comments

@vmanthos
Copy link
Contributor

Before submitting an issue please check that you’ve completed the following steps:

  • Made sure you’re on the latest version ✅
  • Used the search feature to ensure that the bug hasn’t been reported before ✅

Describe the bug

The following was found on a multisite where the subsites were in subfolders. I haven't checked it, but the same should stand for WordPress subfolder installations.

On Kinsta, who do their own CDN rewriting, CSS/JavaScript combination fails when the site's URL contains a path, e.g. https://www.example.com/path/.

The following check is failing:

This is because in the following line:

return str_replace( $cdn_url, $site_url, $url );

we are using only the host:

$site_url = $site_url_parts['scheme'] . '://' . $site_url_parts['host'];

Eventually the $file doesn't contain the root directory path here:

$file = str_replace( $root_url, $root_dir, $url );

This returns false and assets aren't minified/combined.

To Reproduce

Steps to reproduce the behavior:

  1. Create a multisite and add a subfolder subsite.
  2. Add some dummy links to assets to emulate Kinsta's rewriting.
  3. Enable CDN and use the CNAME you used in the dummy links.
  4. Enable CSS/JavaScript minification/combination.
  5. Check the site to see if the minification/combination is working.

Expected behavior

CSS/JavaScript combination should work regardless of the site's URL containing a path when using Kinsta or any host that does CDN rewriting.

Additional context

Related ticket: https://secure.helpscout.net/conversation/1272259848/191944?folderId=2135277

Backlog Grooming (for WP Media dev team use only)

  • Reproduce the problem
  • Identify the root cause
  • Scope a solution
  • Estimate the effort
@arunbasillal arunbasillal added 3rd party compatibility Issues related to 3rd party compatibility like theme, plugin or hosting module: file optimization needs: grooming priority: medium Issues which are important, but no one will go out of business. type: bug Indicates an unexpected problem or unintended behavior labels Sep 17, 2020
@codetipi
Copy link

codetipi commented Nov 9, 2020

Any updates on this? It's quite a bad bug :/

@alfonso100
Copy link
Contributor

The customer opened a new ticket about this case:
https://secure.helpscout.net/conversation/1332879329/210266?folderId=2683093

@engahmeds3ed
Copy link
Contributor

props to @alfonso100

we can solve this issue by replacing the following line

$site_url = $site_url_parts['scheme'] . '://' . $site_url_parts['host'];

with the following line

$site_url = untrailingslashit( $site_url_parts['scheme'] . '://' . $site_url_parts['host'] . $site_url_parts['path'] );

it should do the trick, I tried that on one of our customer's sites and it seems working.

@engahmeds3ed engahmeds3ed added GROOMING IN PROGRESS Use this label when the issue is currently being groomed. and removed needs: grooming labels Dec 16, 2020
@engahmeds3ed
Copy link
Contributor

additional context till finishing grooming:
Kinsta ( and I think BunnyCDN ) when using CDN it removes the subdirectory path from the assets urls so when replacing the site url with the content path it gives not correct path so what we did here is to add the path also to the site url

for example:
main url: https://example.org/sub/
asset url without cdn: https://example.org/sub/wp-content/themes/test/style.css
asset url: https://mk0runnerslab22n01ig.kinstacdn.com/wp-content/themes/test/style.css
so we try to replace https://mk0runnerslab22n01ig.kinstacdn.com/ with https://example.org/ assuming that the asset url has the subdirectory on it (but at kinsta it isn't) so the final url is: https://example.org/wp-content/themes/test/style.css and this is not correct.

@engahmeds3ed engahmeds3ed added needs: grooming and removed GROOMING IN PROGRESS Use this label when the issue is currently being groomed. labels Dec 17, 2020
@vmanthos
Copy link
Contributor Author

Related ticket: https://secure.helpscout.net/conversation/1367510564/223237/

@engahmeds3ed have already confirmed the fix there.

@alfonso100
Copy link
Contributor

related ticket: https://secure.helpscout.net/conversation/1546001981/272321?folderId=2683093
@engahmeds3ed proposed fix also worked there.

Ahmed modified our helper plugin to use a different filter: https://jmp.sh/Uh8kkLj

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3rd party compatibility Issues related to 3rd party compatibility like theme, plugin or hosting module: combine CSS module: combine JS module: minify CSS module: minify JS priority: medium Issues which are important, but no one will go out of business. severity: major Feature is not working as expected and no work around available type: bug Indicates an unexpected problem or unintended behavior
Projects
None yet
Development

No branches or pull requests

7 participants