minify icon indicating copy to clipboard operation
minify copied to clipboard

Fix #329 Prevent regex from getting stripped as comment

Open enricodias opened this issue 5 years ago • 5 comments

This a quick fix that will solve some issues but prevent the removal of single line comments starting with "//i". I guess it's better to leave some comments than breaking the code.

Fixes #323 Fixes #329

enricodias avatar Apr 25 '20 04:04 enricodias

As mentioned in https://github.com/wp-media/wp-rocket/issues/2974, lines with //. should also not be treated as comments. I updated the fix to include this case.

enricodias avatar Dec 19 '20 16:12 enricodias

This change causes side effects in cases of comments starting with //i or //.. They end up being preserved, and after minification, they end up on the same line as the following code, breaking the expected output.

An example:

function codeAddress(data) {
    "use strict";

    if (data === '')
    return;

    var contentString = '<div id="content">'+
        '<div id="siteNotice">'+
            '</div>'+
        '<div id="bodyContent">'+
            '<p>'+data+'</p>'+
            '</div>'+
        '</div>';
    var infowindow = new google.maps.InfoWindow({
    content: contentString
    });
    geocoder.geocode( { 'address': data}, function(results, status) {
        if (status === google.maps.GeocoderStatus.OK) {
            map.setCenter(results[0].geometry.location);
            var marker = new google.maps.Marker({
                map: map,
                position: results[0].geometry.location,
                icon:  'http://demo.qodeinteractive.com/bridge83/wp-content/themes/bridge/img/pin.png',
                title: data['store_title']
            });
            google.maps.event.addListener(marker, 'click', function() {
                infowindow.open(map,marker);
            });
            //infowindow.open(map,marker);
        }
    });
}

is minified to:

function codeAddress(data){"use strict";if(data==='')
return;var contentString='<div id="content">'+'<div id="siteNotice">'+'</div>'+'<div id="bodyContent">'+'<p>'+data+'</p>'+'</div>'+'</div>';var infowindow=new google.maps.InfoWindow({content:contentString});geocoder.geocode({'address':data},function(results,status){if(status===google.maps.GeocoderStatus.OK){map.setCenter(results[0].geometry.location);var marker=new google.maps.Marker({map:map,position:results[0].geometry.location,icon:'http://demo.qodeinteractive.com/bridge83/wp-content/themes/bridge/img/pin.png',title:data.store_title});google.maps.event.addListener(marker,'click',function(){infowindow.open(map,marker)});//infowindow.open(map,marker)}})}

Notice the preserved comment at the end, and since it's on the same line, the following closing parenthesis/brackets are considered inside the comments, thus breaking the code.

remyperona avatar Jul 22 '21 13:07 remyperona

Interesting... but comments staring with . and i without any space in between are less common than regex expressions containing those symbols.

I can't think of a solution that will work on 100% of the cases. Identifying a comment in JS is not a trivial thing.

enricodias avatar Jul 22 '21 13:07 enricodias

You'd be surprised by the number of times it happens, and in popular script like flexslider.js, used in all WooCommerce installations, or popular WP themes like Bridge.

In the end, for our use case, this change is more impactful than the original way.

I agree identifying comments in JS is complex, especially when RegEx is involved. But the solution should not be to create other side effects while fixing the bug.

remyperona avatar Jul 22 '21 16:07 remyperona

How about this pattern? Could you see any side effects?

$js = "
// comment
infowindow.open(map,marker);
//infowindow.open(map,marker);
//.comment
/* comment
*/
test: [/\sedg\//i],
//test: [/\sedg\//i],
if(b)return w.IGNORE;var c=B(fb(a,\"href\"));b=R(fb(a,\"hostname\"));c=c?/^https?:\/\//.test(c)||/^\/\//.test(c):!1;if(c&&!lc(a)){if(la)return w.TRACK;
";

$pattern = <<<'EOD'
~
(?(DEFINE)
    (?<squoted> ' [^'\n\\]*+ (?: \\. [^'\n\\]* )*+ ' )
    (?<dquoted> " [^"\n\\]*+ (?: \\. [^"\n\\]* )*+ " )
    (?<quoted>  \g<squoted> | \g<dquoted> )

    (?<scomment> // \N* )
    (?<mcomment> /\* [^*]*+ (?: \*+ (?!/) [^*]* )*+ \*/ )
    (?<comment> \g<scomment> | \g<mcomment> )

    (?<pattern> / [^\n/*] [^\n/\\]*+ (?>\\.[^\n/\\]*)* / [gimuy]* ) 
)

(?=[[(:,=/"'])
(?|
    \g<quoted> (*SKIP)(*FAIL)
  |
    ( [[(:,=] \s* ) (*SKIP) (?: \g<comment> \s* )*+ ( \g<pattern> )
  | 
    ( \g<pattern> \s* ) (?: \g<comment> \s* )*+ 
    ( \. \s* ) (?:\g<comment> \s* )*+ ([A-Za-z_]\w*)
  |
    \g<comment>
)
~x
EOD;

$js = preg_replace($pattern, '$8$9${10}', $js);

echo $js;

enricodias avatar Jul 23 '21 14:07 enricodias