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

Fix #329 Prevent regex from getting stripped as comment #330

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

enricodias
Copy link
Contributor

@enricodias enricodias commented Apr 25, 2020

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
Copy link
Contributor Author

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

@remyperona
Copy link
Contributor

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.

@enricodias
Copy link
Contributor Author

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.

@remyperona
Copy link
Contributor

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.

@enricodias
Copy link
Contributor Author

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;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regex gets stripped as comment in JS.php Uncaught SyntaxError: Invalid regular expression: missing /
2 participants