? ? Pending

User tests: Successful: Unsuccessful:

avatar PhilETaylor
PhilETaylor
18 Dec 2020

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.

avatar PhilETaylor PhilETaylor - open - 18 Dec 2020
avatar PhilETaylor PhilETaylor - change - 18 Dec 2020
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 18 Dec 2020
Category Administration com_finder
avatar PhilETaylor PhilETaylor - change - 18 Dec 2020
Title
use StringHelper::substr method from instead of \substr
[3.9.24RC] use StringHelper::substr method from instead of \substr
avatar PhilETaylor PhilETaylor - edited - 18 Dec 2020
avatar HLeithner HLeithner - change - 18 Dec 2020
Title
[3.9.24RC] use StringHelper::substr method from instead of \substr
[3.9] use StringHelper::substr method from instead of \substr
avatar HLeithner HLeithner - edited - 18 Dec 2020
avatar HLeithner
HLeithner - comment - 18 Dec 2020

Unrelated to 3.9.24-rc so I removed it from the title, anyway thanks for the fix

avatar infograf768
infograf768 - comment - 19 Dec 2020

I have tested this item successfully on 59f6557

makes sense. OK on review.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/31722.

avatar infograf768 infograf768 - test_item - 19 Dec 2020 - Tested successfully
avatar richard67
richard67 - comment - 19 Dec 2020

I have tested this item successfully on 59f6557

Code review.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/31722.

avatar richard67 richard67 - test_item - 19 Dec 2020 - Tested successfully
avatar richard67 richard67 - change - 19 Dec 2020
Status Pending Ready to Commit
avatar richard67
richard67 - comment - 19 Dec 2020

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/31722.

avatar richard67 richard67 - change - 19 Dec 2020
Labels Added: ? ?
avatar infograf768
infograf768 - comment - 19 Dec 2020

@HLeithner
Milestoned to 3.9.24. Let you merge.

avatar SharkyKZ
SharkyKZ - comment - 19 Dec 2020

I have tested this item ? unsuccessfully on 59f6557

Code review.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/31722.

avatar SharkyKZ SharkyKZ - test_item - 19 Dec 2020 - Tested unsuccessfully
avatar PhilETaylor
PhilETaylor - comment - 19 Dec 2020

@SharkyKZ I have tested this item ? unsuccessfully on 59f6557

??

avatar richard67
richard67 - comment - 19 Dec 2020

@SharkyKZ What's wrong?

avatar HLeithner
HLeithner - comment - 19 Dec 2020

2 things

  1. first with this pr all language which needs utf8 chars are broken. reason for this is thats the PR is incomplete because the strrpos doesn't use utf8 version
  2. the PR is not needed because we only search for a whitespace 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

avatar HLeithner HLeithner - close - 19 Dec 2020
avatar HLeithner HLeithner - change - 19 Dec 2020
Status Ready to Commit Closed
Closed_Date 0000-00-00 00:00:00 2020-12-19 13:10:40
Closed_By HLeithner
Labels
avatar PhilETaylor
PhilETaylor - comment - 19 Dec 2020

Cool - that saved my Sunday afternoon. Thanks.

avatar richard67
richard67 - comment - 19 Dec 2020

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.

avatar infograf768
infograf768 - comment - 19 Dec 2020

My mistake too. Thanks.

Add a Comment

Login with GitHub to post a comment