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

substr_replace works incorrectly with utf-8 #154

Closed
danyaPostfactum opened this Issue May 8, 2018 · 12 comments

Comments

4 participants
@danyaPostfactum

danyaPostfactum commented May 8, 2018

substr_replace inserts string at wrong place under certain conditions. I found that autoptimize breaks my html. It looks like substr_replace works bad with UTF-8. You can see on the screenshot that <script> is inserted at wrong place.
1525757520075

I fixed this bug by replacing builtin substr_replace with custom mb_substr_replace:

function mb_substr_replace($original, $replacement, $position, $length)
{
 $startString = mb_substr($original, 0, $position, "UTF-8");
 $endString = mb_substr($original, $position + $length, mb_strlen($original), "UTF-8");

 $out = $startString . $replacement . $endString;

 return $out;
}

I can provide my html before substr_replace for testing if you need. But I guess this bug depends on server configuration.

@zytzagoo

This comment has been minimized.

Show comment
Hide comment
@zytzagoo

zytzagoo May 9, 2018

Collaborator

Hi, thanks for taking the time to report the issue and the fix you applied!

Please, do provide the html before substr_replace is taking place.

Is this happening with default settings/setup, or are you using some additional autoptimize filters to modify some other things too?

Collaborator

zytzagoo commented May 9, 2018

Hi, thanks for taking the time to report the issue and the fix you applied!

Please, do provide the html before substr_replace is taking place.

Is this happening with default settings/setup, or are you using some additional autoptimize filters to modify some other things too?

@zytzagoo

This comment has been minimized.

Show comment
Hide comment
@zytzagoo

zytzagoo May 9, 2018

Collaborator

Just cross-referencing this, since it's related: https://wordpress.org/support/topic/weird-bug-with-autoptimize-script-insertion/

If #155 is too much, perhaps we should add an entry to the FAQ related to this, mentioning the php.ini setting/change that can solve this without all the extra code:

mbstring.func_overload = 2
mbstring.internal_encoding = UTF-8
Collaborator

zytzagoo commented May 9, 2018

Just cross-referencing this, since it's related: https://wordpress.org/support/topic/weird-bug-with-autoptimize-script-insertion/

If #155 is too much, perhaps we should add an entry to the FAQ related to this, mentioning the php.ini setting/change that can solve this without all the extra code:

mbstring.func_overload = 2
mbstring.internal_encoding = UTF-8
@futtta

This comment has been minimized.

Show comment
Hide comment
@futtta

futtta May 9, 2018

Owner

Are you sure the reporter in https://wordpress.org/support/topic/weird-bug-with-autoptimize-script-insertion/ is adding that to his php.ini? Or is he removing it?

If he is removing it; super and then indeed we should go the FAQ way. If he added it, then we'll have to add your new code, because mbstring.func_overload is deprecated and will get removed at some point .. :-/

Owner

futtta commented May 9, 2018

Are you sure the reporter in https://wordpress.org/support/topic/weird-bug-with-autoptimize-script-insertion/ is adding that to his php.ini? Or is he removing it?

If he is removing it; super and then indeed we should go the FAQ way. If he added it, then we'll have to add your new code, because mbstring.func_overload is deprecated and will get removed at some point .. :-/

@zytzagoo

This comment has been minimized.

Show comment
Hide comment
@zytzagoo

zytzagoo May 9, 2018

Collaborator

must be adding, beacuse that way strpos starts working properly for multibyte strings...

without overload, strpos gets wrong position if there are multibyte chars present preceding the searched string...

Collaborator

zytzagoo commented May 9, 2018

must be adding, beacuse that way strpos starts working properly for multibyte strings...

without overload, strpos gets wrong position if there are multibyte chars present preceding the searched string...

@futtta

This comment has been minimized.

Show comment
Hide comment
@futtta

futtta May 10, 2018

Owner

let try pinging him here to confirm; @Enuriru are you the same person who reported https://wordpress.org/support/topic/weird-bug-with-autoptimize-script-insertion/? And if so; can you confirm you added those 2 lines to your php.ini?

(and if not; sorry for bothering you :-) )

Owner

futtta commented May 10, 2018

let try pinging him here to confirm; @Enuriru are you the same person who reported https://wordpress.org/support/topic/weird-bug-with-autoptimize-script-insertion/? And if so; can you confirm you added those 2 lines to your php.ini?

(and if not; sorry for bothering you :-) )

@Enuriru

This comment has been minimized.

Show comment
Hide comment
@Enuriru

Enuriru May 10, 2018

Enuriru commented May 10, 2018

@futtta

This comment has been minimized.

Show comment
Hide comment
@futtta

futtta May 10, 2018

Owner

thanks for the feedback @Enuriru !

guess we have no choice but to bite the bullet @zytzagoo :-)

Owner

futtta commented May 10, 2018

thanks for the feedback @Enuriru !

guess we have no choice but to bite the bullet @zytzagoo :-)

@futtta

This comment has been minimized.

Show comment
Hide comment
@futtta

futtta May 10, 2018

Owner

OK, so @Enuriru and @danyaPostfactum :

As mbstring.func_overload = 2 is deprecated we cannot rely on this for future use (guess as of PHP 7.3), so the code changes by @zytzagoo should make AO work for you even with that function and is more robust then the change @danyaPostfactum made. Could you download the beta zipfile and test to confirm (or not)?

Owner

futtta commented May 10, 2018

OK, so @Enuriru and @danyaPostfactum :

As mbstring.func_overload = 2 is deprecated we cannot rely on this for future use (guess as of PHP 7.3), so the code changes by @zytzagoo should make AO work for you even with that function and is more robust then the change @danyaPostfactum made. Could you download the beta zipfile and test to confirm (or not)?

@futtta

This comment has been minimized.

Show comment
Hide comment
@futtta

futtta Jun 5, 2018

Owner

any feedback @Enuriru or @danyaPostfactum ? :-)

Owner

futtta commented Jun 5, 2018

any feedback @Enuriru or @danyaPostfactum ? :-)

@danyaPostfactum

This comment has been minimized.

Show comment
Hide comment
@danyaPostfactum

danyaPostfactum Jun 5, 2018

Unfortunately I can't check, I have no longer access to the website..

danyaPostfactum commented Jun 5, 2018

Unfortunately I can't check, I have no longer access to the website..

@futtta

This comment has been minimized.

Show comment
Hide comment
@futtta

futtta Jun 5, 2018

Owner

no problem @danyaPostfactum , thanks for the feedback anyway! 👍

and if you have some time, feel free to take Autoptimize 2.4 beta for a spin elsewhere! :-)

Owner

futtta commented Jun 5, 2018

no problem @danyaPostfactum , thanks for the feedback anyway! 👍

and if you have some time, feel free to take Autoptimize 2.4 beta for a spin elsewhere! :-)

@futtta

This comment has been minimized.

Show comment
Hide comment
@futtta

futtta Oct 7, 2018

Owner

Closing as fixed with AO24 release, although for 2.4.1 this will be off by default (but can be turned on with a filter provided mb_* functions are available).

Owner

futtta commented Oct 7, 2018

Closing as fixed with AO24 release, although for 2.4.1 this will be off by default (but can be turned on with a filter provided mb_* functions are available).

@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