[#31722] - [3.9] use StringHelper::substr method from instead of \substr
- Closed
- 19 Dec 2020
- Medium
- Build: staging
- # 31722
- Diff
- PhilETaylor:patch-24
User tests: Successful: Unsuccessful:
Summary of Changes
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
Testing Instructions
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.
Actual result BEFORE applying this Pull Request
Im guessing the handling of Cyrillic and other alphabets might be odd when using \substr as opposed to StringHelper::substr
Expected result AFTER applying this Pull Request
Everything works
Documentation Changes Required
None.
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Category | ⇒ | Administration com_finder |
| Title |
|
||||||
| Title |
|
||||||
I have tested this item
makes sense. OK on review.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/31722.
I have tested this item
Code review.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/31722.
| Status | Pending | ⇒ | Ready to Commit |
RTC
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/31722.
| Labels |
Added:
?
?
|
||
@HLeithner
Milestoned to 3.9.24. Let you merge.
I have tested this item
Code review.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/31722.
2 things
- first with this pr all language which needs utf8 chars are broken. reason for this is thats the PR is incomplete because the
strrposdoesn't use utf8 version - the PR is not needed because we only search for a whitespace
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