User tests: Successful: Unsuccessful:
Reviewed in testing 3.9.24RC PRs... Change made on sight of code review.
Hat tip to @Avalon18 who highlighted in #31533 that com_finder did not always manipulate strings (specifically Cyrillic) correctly.
This PR is just expounding the fix
Code review, maybe someone intimate with com_finder might be able to give a test case that triggers these lines of code? I only edited on Github quickly.
Im guessing the handling of Cyrillic and other alphabets might be odd when using \substr as opposed to StringHelper::substr
Everything works
None.
Status | New | ⇒ | Pending |
Category | ⇒ | Administration com_finder |
Title |
|
Title |
|
I have tested this item
makes sense. OK on review.
I have tested this item
Code review.
Status | Pending | ⇒ | Ready to Commit |
RTC
Labels |
Added:
?
?
|
@HLeithner
Milestoned to 3.9.24. Let you merge.
I have tested this item
Code review.
2 things
strrpos
doesn't use utf8 version
which is not part of the utf8 range.A simple tests confirms this:
$buffer = "test abc";
$buffer = "äöü ßaäö";
// the strings are from news pages I have no idea what's it means
$buffer = " помощник главы Азербайджана";
$buffer = "ولم يعرف سبب الانفجار. ويقول";
$buffer = "民航處處長 刑事檢控專員";
$ls1 = strrpos($buffer, ' ');
$string1 = substr($buffer, 0, $ls1);
$buffer1 = StringHelper::trim(substr($buffer, $ls1));
$ls2 = StringHelper::strrpos($buffer, ' ');
$string2 = StringHelper::substr($buffer, 0, $ls2);
$buffer2 = StringHelper::trim(StringHelper::substr($buffer, $ls2));
var_dump([$buffer, $ls1, $string1, $buffer1, $ls2, $string2, $buffer2]);
#31533 fix a complete different use case.
I'm closing this PR because it's not needed and adds performance penalty ymmv
Status | Ready to Commit | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2020-12-19 13:10:40 |
Closed_By | ⇒ | HLeithner | |
Labels |
Cool - that saved my Sunday afternoon. Thanks.
Silly me that I haven't seen that. I should have looked deeper into it. Sorry, and thanks Sharky and Harald for pointing on it.
My mistake too. Thanks.
Unrelated to 3.9.24-rc so I removed it from the title, anyway thanks for the fix