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

3.7: External CSS Minify exclusions are not working as intended #3118

Closed
4 tasks done
webtrainingwheels opened this issue Sep 17, 2020 · 3 comments · Fixed by #3300
Closed
4 tasks done

3.7: External CSS Minify exclusions are not working as intended #3118

webtrainingwheels opened this issue Sep 17, 2020 · 3 comments · Fixed by #3300
Assignees
Labels
effort: [S] 1-2 days of estimated development time module: file optimization priority: medium Issues which are important, but no one will go out of business. type: bug Indicates an unexpected problem or unintended behavior
Milestone

Comments

@webtrainingwheels
Copy link

webtrainingwheels commented Sep 17, 2020

The specs for excluding an external CSS file from minify are:

3rd Party: Use either the full URL path or only the domain name, to exclude external CSS.

However there are some cases where this is not working as expected.

Samples:

Seems only to be an issue with Minify, not Combine

Tickets:

To Reproduce
Steps to reproduce the behavior:

  1. Include one of the above URLS in the HTML of a site
  2. Enable Minify CSS
  3. Try to exclude the file with either the domain or full URL

Expected behavior
The files should be excluded using either the domain or full URL

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 module: file optimization needs: grooming priority: low Issues that can wait type: bug Indicates an unexpected problem or unintended behavior labels Sep 18, 2020
@vmanthos
Copy link
Contributor

Related ticket: https://secure.helpscout.net/conversation/1310458122/202765?folderId=2135277

I further investigated this.

This is because of the preg_match() we run here:

if ( preg_match( '#^(' . $this->excluded_files . ')$#', $file_path ) ) {

If the excluded file is e.g. cdn.jsdelivr.net/npm/bootstrap@4.5.3/dist/css/bootstrap.min.css, the $file_path is /npm/bootstrap@4.5.3/dist/css/bootstrap.min.css, and preg_match() returns 0.

Hence the file is not excluded and is cached locally.

@vmanthos
Copy link
Contributor

@iCaspar
Copy link
Contributor

iCaspar commented Nov 6, 2020

  • Can Reproduce

  • Identified cause: per @vmanthos, the regex fails to identify domain names. [X]

Scope Solution:

  • Add testcases for domain names and full URLs to fixtures for inc/Engine/Optimization/Minify/CSS/Minify
  • Make tests pass by adjusting regex for preg_match() in AbsstractCSSOptimization::is_minify_excluded_file()

Estimated Effort:

  • [S]

@Tabrisrp Tabrisrp added the effort: [S] 1-2 days of estimated development time label Nov 6, 2020
@arunbasillal arunbasillal added this to the 3.7.6 milestone Nov 10, 2020
@iCaspar iCaspar self-assigned this Nov 13, 2020
@Tabrisrp Tabrisrp linked a pull request Nov 16, 2020 that will close this issue
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: file optimization priority: medium Issues which are important, but no one will go out of business. type: bug Indicates an unexpected problem or unintended behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants