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

Fixed replace modifier by converting encoding if needed #590

Closed
wants to merge 2 commits into from

Conversation

AnrDaemon
Copy link
Contributor

mb_split will fail if $pattern or $string contains byte sequences not valid for mb_regex_encoding().

Convert both strings and set regex encoding before calling mb_split() if needed.

Fixes #549

@AnrDaemon
Copy link
Contributor Author

Now, this should be more robust. Turned out, not every UNICODE encoding supported by mbstring regex is equally supported by preg_quote().

I've considered using preg_split() instead of mb_split(), but ultimately decided against it in favor of inherent validation in mb_split().


/** @var array Source data for tests, hexed to protect from accidental reencoding */
private $data = array(
"subject" => '4772c3bc6e6577616c64', // "Grünewald"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add "regex sensitive" characters to the test strings just to prove that your regex quoting and splitting works.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean, characters like {} ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These should be added to parent tests, I presume. Since this is regression only test.

Copy link

@cmanley cmanley Jul 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes characters like that and: . + * \ / ? ( ) [ ]
Thanks.

@matslindh
Copy link

Bump. This fixes replace borking strings on any site still using alternative charsets, such as iso-8859-1.

`mb_split` will fail if `$pattern` or `$string` contains byte sequences not valid for `mb_regex_encoding()`.

Convert both strings and set regex encoding before calling `mb_split()` if needed.

Fixes smarty-php#549
@AnrDaemon
Copy link
Contributor Author

AnrDaemon commented Mar 4, 2022

Opened a related PR #740 against 3.1 maintenance branch.

@AnrDaemon
Copy link
Contributor Author

Related patch #740 into 3.1 branch has been committed recently. Please review this one too.

@AnrDaemon
Copy link
Contributor Author

@wisskid did you backport these changes manually? If so, feel free to close this PR.

@wisskid
Copy link
Contributor

wisskid commented Aug 23, 2022

@wisskid did you backport these changes manually? If so, feel free to close this PR.

@AnrDaemon yes, I did. 20a8026

@wisskid wisskid closed this Aug 23, 2022
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.

problems with smarty_mb_str_replace PHP >= 7.1
4 participants