JS Minification - Uncode: abnormal string added when excluding a file

DahmaniAdame commented 3 years ago

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

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

Describe the bug When trying to exclude the following file on Uncode /wp-content/themes/uncode/library/js/ai-uncode.js, the following string appends to it '%20id='uncodeAI'%20data-home='/'%20data-path='/'%20data-breakpoints-images='' id='uncodeAI' data-home='/' data-path='/' data-breakpoints-images=' on each save.

After many saves, this ultimately results in Compilation failed: regular expression is too large.

[Wed Jun 02 10:24:59.356558 2021] [error] [pid 75278] mod_proxy_fcgi.c(871): [client x.x.x.x:xxxx] XXX: Got error 'PHP message: PHP Warning: preg_match(): Compilation failed: regular expression is too large at offset 226797
/www/wp-content/plugins/wp-rocket/inc/Engine/Optimization/Minify/JS/AbstractJSOptimization.php on line 121
PHP message: PHP Warning: preg_match(): Compilation failed: regular expression is too large

Likely a Regex issue when extracting the script path:

<script type='text/javascript' src='https://example.com/wp-content/themes/uncode/library/js/ai-uncode.js' id='uncodeAI' data-home='/' data-path='/' data-breakpoints-images='258,516,720,1032,1440,2064,2880'></script>

To Reproduce Steps to reproduce the behavior:

Use Uncode theme
Enable JS minification
Exclude /wp-content/themes/uncode/library/js/ai-uncode.js
See error

Expected behavior We should only add /wp-content/themes/uncode/library/js/ai-uncode.js to the exclusion list.

Screenshots

Additional context Ticket - https://secure.helpscout.net/conversation/1531260252/268617?folderId=3864735 Confirmed on Mega/Uncode

Backlog Grooming (for WP Media dev team use only)

[ ] Reproduce the problem
[ ] Identify the root cause
[ ] Scope a solution
[ ] Estimate the effort

vmanthos commented 3 years ago

An additional note to what @DahmaniAdame wrote.

That file is among those we automatically exclude for Uncode, and therefore there isn't a need to be added in the exclusion text area: https://github.com/wp-media/wp-rocket/blob/59045e5ace9948b5ad844c6f4d5c46122089af7b/inc/3rd-party/themes/uncode.php#L20

Also, the same issue will occur when excluding any of the files there, e.g.:

/library/js/min/ai-uncode.min.js

mostafa-hisham commented 3 years ago

Reproduce the issue ✅

Reproduced on local test site

Identify the root cause ✅

there are two problems here

First Problem

The Uncode them use this filter to add extra attributes to the script tag that contains "ai-uncode" add_filter('clean_url','uncode_unclean_url',10,3);

function uncode_unclean_url( $good_protocol_url, $original_url, $_context){
...
return apply_filters( 'uncode_ai_script_path', $url_parts['path'], $url_parts ) . "' " . $data_mobile_advanced
"id='uncodeAI'".$ai_async." data-home='".$url_home."' data-path='".$path_domain."' data-breakpoints-images='" . $ai_sizes;
...
}

in "themes/uncode/core/inc/main.php"

so when esc_url() wp function is called the extra attributes will be added to the URL

Second Problem

"rocket_is_internal_file" not working correctly with internal files without the domain

https://github.com/wp-media/wp-rocket/blob/fa37d72c18f50702dd98d6d38f0be48ff2446965/inc/functions/formatting.php#L129-L162

when you add "/library/js/min/ai-uncode.min.js" in exclude text area this function "rocket_is_internal_file" will return that the file is external

and the First Problem will happen because the function "rocket_validate_css" will use "esc_url_raw" that adds the extra Encode theme attributes

https://github.com/wp-media/wp-rocket/blob/fa37d72c18f50702dd98d6d38f0be48ff2446965/inc/functions/formatting.php#L93-L103

Scope a solution ✅

we can add this this function "is_external_path" in "inc/Engine/Optimization/CSSTrait.php" in "wp-content/plugins/wp-rocket/inc/functions/formatting.php"

https://github.com/wp-media/wp-rocket/blob/fa37d72c18f50702dd98d6d38f0be48ff2446965/inc/Engine/Optimization/CSSTrait.php#L433-L480

and use it instead of "rocket_is_internal_file"

this will fix the second problem and this Issue because the file will be treated as an internal resource

Estimate the effort ✅

Effort [XS]

piotrbak commented 4 months ago

There's no feedback from customers regarding this one. It's not going to be implemented in the near future. We're open to discuss and reopen the issue though.

wp-media / wp-rocket