Repository navigation
[Grapheme] Fix grapheme_strpos() and friends matching inside a grapheme cluster - #719
Merged
Merged
Conversation
…me cluster The intl extension does not find "q" in "q̃" because the match would split a grapheme cluster. A match can still end or start inside a cluster when what it leaves out is only characters like U+200C that the extension ignores.
nicolas-grekas
added a commit
to symfony/symfony
that referenced
this pull request
Oct 15, 2026
… ext-intl (nicolas-grekas) This PR was merged into the 6.4 branch. Discussion ---------- [String] Fix UnicodeString::replace() and split() without ext-intl | Q | A | ------------- | --- | Branch? | 6.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Issues | - | License | MIT Follow-up of #66969, which broke the [x86 / minimal-exts job](https://github.com/symfony/symfony/actions/runs/38707200364/job/116329436616), where ext-intl is not loaded. Without ext-intl, `grapheme_strpos()` reports a needle that starts inside a grapheme at the next grapheme. So `u("می\u{200C}خواهم")->replace("\u{200C}", ' ')` didn't find the ZWNJ. This PR also searches the needle from the previous grapheme in that case. The polyfill also finds "q" inside "q̃", so this test case now runs only with ext-intl. The polyfill is fixed in symfony/polyfill#719. Commits ------- 81c0538 [String] Fix UnicodeString::replace() and split() without ext-intl
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Found while fixing symfony/symfony#66969 without ext-intl.
The polyfills of
grapheme_str[ri]?pos()use plainstrpos(), so they find "q" inside "q̃" where the extension returnsfalse:grapheme_strpos(" q\u{0303}", 'q')false1grapheme_strpos("q\u{0303}", "\u{0303}")false1grapheme_strpos("\u{1F44D}\u{1F3FB}", "\u{1F44D}")false0A match is now rejected when it splits a grapheme cluster. The extension still accepts a match that leaves out only characters it ignores, like U+200C in Persian text, so the polyfill does the same. The list of these characters comes from ICU 74.
On 100k random strings, the polyfill now disagrees with the extension half as often on whether there's a match. Most of the remaining differences come from the ZWJ simplification of the cluster regexp used when PCRE < 10.44. That simplification also makes about 500 of these cases disagree where they agreed before.
When the match starts and ends next to ASCII characters, the cost is unchanged. Otherwise the lookup is about 2x slower, with no extra memory.