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

autoptimizeBase.php str_is_valid_regex throwing fatal error #173

Closed
futtta opened this Issue Jul 4, 2018 · 10 comments

Comments

3 participants
@futtta
Owner

futtta commented Jul 4, 2018

reported by beta-user;

  • only happens when browsing on internal URL and when HTTP auth is active
  • the ImagickException makes this weird
  • happens since beta-3 and is also the case with most recent archive
  • asked to rollback to beta-2 to see if this is a regression
Fatal error: Uncaught ImagickException: preg_match(): No ending matching delimiter '>' found in /nas/content/staging/zerocatersite2/wp-content/plugins/autoptimize/classes/autoptimizeBase.php:558 

Stack trace: 
#0 /nas/content/staging/zerocatersite2/wp-content/plugins/autoptimize/classes/autoptimizeBase.php(558): preg_match('<!--[if', '') 
#1 /nas/content/staging/zerocatersite2/wp-content/plugins/autoptimize/classes/autoptimizeBase.php(583): autoptimizeBase->str_is_valid_regex('<!--[if') 
#2 /nas/content/staging/zerocatersite2/wp-content/plugins/autoptimize/classes/autoptimizeBase.php(232): autoptimizeBase->replace_contents_with_marker_if_exists('IEHACK', '<!--[if', '#<!--\\[if.*?\\[e...', '<!doctype html>...') 
#3 /nas/content/staging/zerocatersite2/wp-content/plugins/autoptimize/classes/autoptimizeScripts.php(132): autoptimizeBase->hide_iehacks('<!doctype html>...') 
#4 /nas/content/staging/zerocatersite2/wp-content/plugins/autoptimize/classes/autoptimizeMain.php(431): autoptimizeScripts->read(Array) 
#5 [internal function]: in /nas/content/staging/zerocatersite2/wp-content/plugins/autoptimize/classes/autoptimizeBase.php on line 558```

@futtta futtta changed the title from autoptimizeBase.php str_is_valid_regex throwingn fatal error to autoptimizeBase.php str_is_valid_regex throwing fatal error Jul 4, 2018

@zytzagoo

This comment has been minimized.

Show comment
Hide comment
@zytzagoo

zytzagoo Jul 4, 2018

Collaborator

interesing! any chance of getting our hands on the (ideally exact/complete) minimal markup that triggers this?

(since i’m assuming it’s not just due to iehacks being present somewhere in any kind of markup, or this would blow up a lot sooner i the wild?)

Collaborator

zytzagoo commented Jul 4, 2018

interesing! any chance of getting our hands on the (ideally exact/complete) minimal markup that triggers this?

(since i’m assuming it’s not just due to iehacks being present somewhere in any kind of markup, or this would blow up a lot sooner i the wild?)

@zytzagoo

This comment has been minimized.

Show comment
Hide comment
@zytzagoo

zytzagoo Jul 6, 2018

Collaborator

What are the odds of cbb80e7 having an effect on this?

I mean, that commit is now a departure from "the way things were" conceptually, if compared to current master (2.3.4) -- first iehacks were restored, then comments, and it was the same in both the beta and master branches: https://github.com/futtta/autoptimize/blob/master/classes/autoptimizeStyles.php#L472

I guess it will depend on the exact markup etc., but I guess the same issue/fix should be present in 2.3.4/master then too? If not, why not?

Or we need to dig a lot deeper.

Collaborator

zytzagoo commented Jul 6, 2018

What are the odds of cbb80e7 having an effect on this?

I mean, that commit is now a departure from "the way things were" conceptually, if compared to current master (2.3.4) -- first iehacks were restored, then comments, and it was the same in both the beta and master branches: https://github.com/futtta/autoptimize/blob/master/classes/autoptimizeStyles.php#L472

I guess it will depend on the exact markup etc., but I guess the same issue/fix should be present in 2.3.4/master then too? If not, why not?

Or we need to dig a lot deeper.

zytzagoo added a commit to zytzagoo/autoptimize that referenced this issue Jul 6, 2018

moves str_is_valid_regex() to autoptimizeUtils and adds few basic tes…
…ts for it

this hopefully proves it doesn't explode in the general case of being given
an apparently un-closed regex or similar. ref: futtta#173

futtta added a commit that referenced this issue Jul 6, 2018

moves str_is_valid_regex() to autoptimizeUtils and adds few basic tes…
…ts for it (#174)

this hopefully proves it doesn't explode in the general case of being given
an apparently un-closed regex or similar. ref: #173
@futtta

This comment has been minimized.

Show comment
Hide comment
@futtta

futtta Jul 6, 2018

Owner

re cbb80e7; theoretically this fix should also go in 2.3.4, but I consider the master branch frozen as I will not push out another 2.3.x version.

but i do consider the order of restoring in autoptimizeStyles wrong in all but current 2.4, because the order for hiding is:

  1. hide noptimize
  2. hide (no)scirpt
  3. hide iehacks
  4. hide comments

unhiding (restoring) should logically be done in the exact opposite way:

  1. unhide comments
  2. unhide iehacks
  3. unhide (no)script
  4. unhidenoptimize

which was not done so before. let's await the OP's feedback; he promised he would revert to 2.4-beta2 to see if the problem goes away. if it does we'll have to dig deeper, but I do feel the order of restoring in aoStyles was wrong before and is right now.

Owner

futtta commented Jul 6, 2018

re cbb80e7; theoretically this fix should also go in 2.3.4, but I consider the master branch frozen as I will not push out another 2.3.x version.

but i do consider the order of restoring in autoptimizeStyles wrong in all but current 2.4, because the order for hiding is:

  1. hide noptimize
  2. hide (no)scirpt
  3. hide iehacks
  4. hide comments

unhiding (restoring) should logically be done in the exact opposite way:

  1. unhide comments
  2. unhide iehacks
  3. unhide (no)script
  4. unhidenoptimize

which was not done so before. let's await the OP's feedback; he promised he would revert to 2.4-beta2 to see if the problem goes away. if it does we'll have to dig deeper, but I do feel the order of restoring in aoStyles was wrong before and is right now.

@futtta

This comment has been minimized.

Show comment
Hide comment
@futtta

futtta Jul 23, 2018

Owner

Just got confirmation from OP that the issue also occurs on AO 2.4-beta2 (so before cbb80e7).

Owner

futtta commented Jul 23, 2018

Just got confirmation from OP that the issue also occurs on AO 2.4-beta2 (so before cbb80e7).

@zytzagoo

This comment has been minimized.

Show comment
Hide comment
@zytzagoo

zytzagoo Jul 23, 2018

Collaborator

And the same error/exception happens with the most recent beta tree download/zip/checkout?

Is there a chance of running/installing the AO test suite in that environment to see if the simple tests for str_is_valid_regex() also blow up? (or perhaps they end up running fine, which could also help eliminate some things...)

I kind of feel like the environment has some extra error handling manipulations in there, which is why we're seeing weird/unrelated things in the initial trace, but without a lot more details about the env, it's impossible to say anything concrete...

Collaborator

zytzagoo commented Jul 23, 2018

And the same error/exception happens with the most recent beta tree download/zip/checkout?

Is there a chance of running/installing the AO test suite in that environment to see if the simple tests for str_is_valid_regex() also blow up? (or perhaps they end up running fine, which could also help eliminate some things...)

I kind of feel like the environment has some extra error handling manipulations in there, which is why we're seeing weird/unrelated things in the initial trace, but without a lot more details about the env, it's impossible to say anything concrete...

@yasirahmedsiddiqui

This comment has been minimized.

Show comment
Hide comment
@yasirahmedsiddiqui

yasirahmedsiddiqui Jul 23, 2018

This error keep coming with beta-1 but disappear with AO 2.3.4

yasirahmedsiddiqui commented Jul 23, 2018

This error keep coming with beta-1 but disappear with AO 2.3.4

@futtta

This comment has been minimized.

Show comment
Hide comment
@futtta

futtta Jul 23, 2018

Owner

Thanks @yasirahmedsiddiqui , could you have a look at Zytzagoo's questions as well? :-)

Owner

futtta commented Jul 23, 2018

Thanks @yasirahmedsiddiqui , could you have a look at Zytzagoo's questions as well? :-)

@yasirahmedsiddiqui

This comment has been minimized.

Show comment
Hide comment
@yasirahmedsiddiqui

yasirahmedsiddiqui Jul 23, 2018

if you guys required, i can give you a new install to investigate. the latest beta downloaded 15 days back and have same issue.

yasirahmedsiddiqui commented Jul 23, 2018

if you guys required, i can give you a new install to investigate. the latest beta downloaded 15 days back and have same issue.

@futtta

This comment has been minimized.

Show comment
Hide comment
@futtta

futtta Jul 24, 2018

Owner

@yasirahmedsiddiqui if I remember correctly, this only occurs if;

  • the site has basic auth on
  • it is accessed on the internal IP-range of your hoster

is that correct? if so, can we even test (esp. considering the ip-range limitation)?

Owner

futtta commented Jul 24, 2018

@yasirahmedsiddiqui if I remember correctly, this only occurs if;

  • the site has basic auth on
  • it is accessed on the internal IP-range of your hoster

is that correct? if so, can we even test (esp. considering the ip-range limitation)?

@futtta

This comment has been minimized.

Show comment
Hide comment
@futtta

futtta Oct 7, 2018

Owner

closing due to lack of feedback/ info and no other reports.

Owner

futtta commented Oct 7, 2018

closing due to lack of feedback/ info and no other reports.

@futtta futtta closed this Oct 7, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment