Fix #20268: Minor optimisation in \yii\helpers\BaseArrayHelper::map

[chriscpty]

Q	A
Is bugfix?	❌
New feature?	✔️
Breaks BC?	❌
Fixed issues	#20268

Information re: what this does is in the issue.

In the process, I also improved the ArrayHelper::map() tests a bit to also test cases where callbacks are passed in for the three parameters.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 64.84%. Comparing base (3c75ff1) to head (4ed589f).
Report is 2 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #20269      +/-   ##
============================================
- Coverage     64.95%   64.84%   -0.11%     
- Complexity    11396    11401       +5     
============================================
  Files           430      430              
  Lines         36925    37138     +213     
============================================
+ Hits          23984    24083      +99     
- Misses        12941    13055     +114     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[rob006]

I'm afraid this will change behavior for keys with dots, for exampleArrayHelper::map($array, 'model1.id', 'model2.name'). Old implementation tried to fetch value from $arrayItem['model1']['id'], new one will always use $arrayItem['model1.id'].

[rhertogh]

Please also add a check that both $from and $to don't contain a dot (.).
The ArrayHelper::getValue() function supports getting a value by a "path" string.
Without this check the following code would break:

$array = [
    [
        'prop' => [
            'a' => 'foo',
            'b' => 'bar',
        ],
    ],
];

ArrayHelper::map($array, 'prop.a', 'prop.b');

[samdark]

Yes. It is necessary. @chriscpty would you please redo performance test with such a check?

[chriscpty]

performance with the added check:

before: 1729233314.6667
after: 1729233315.4382
elapsed: 0.77146291732788
before: 1729233315.4382
after: 1729233316.245
elapsed: 0.80677795410156
before: 1729233316.245
after: 1729233317.0443
elapsed: 0.79932498931885
before: 1729233317.0443
after: 1729233317.8304
elapsed: 0.78613996505737
before: 1729233317.8305
after: 1729233318.6256
elapsed: 0.79515385627747
bench size: 18000

I'll add the fix (and related tests) in a second

[chriscpty]

I also added a BaseStringHelper::contains() function as php7 does not have str_contains. This will become redundant once the minimum supported php version is raised to php8.0 or higher.

[rob006]

I also added a BaseStringHelper::contains()

I'm not sure if it is worth to polyfill function, that exists in PHP since 4 years. This method is already redundant for ~70% of framework users.

[chriscpty]

This method is already redundant for ~70% of framework users

if that number is true, that'd mean not including the polyfill would be a breaking change for 30% of users. Yii2's minimum supported php version is still 7.3 as far as I'm aware & I don't want to introduce breaking changes for a minor optimisation.

Whether yii2 should bother supporting PHP versions that are EOL is a separate question that is up to the maintainers to answer.

[rob006]

if that number is true, that'd mean not including the polyfill would be a breaking change for 30% of users.

In what scenario not adding new function would be a BC break? 70% of framework installations is for PHP8 - in that case developer could use str_contains() directly, the rest could stick to strpos() or use actual polyfill from symfony. There is no need to add BaseStringHelper::contains() in order to implement this minor optimization for ArrayHelper::map().

[chriscpty]

so in other words, you'd rather have it as a construct like

if (function_exists('str_contains') ? str_contains([...]) : (mb_strpos([...]) !== -1))

?

I originally considered doing that, but figured it'd make the condition for the optimisation ugly to read.

EDIT: or am I mistunderstanding and what you're saying is "this optimisation is not worth adding a new function, so you'd rather not add it"? I'd get that, but also don't think there's harm in adding it

[rob006]

My suggestion is:

1. Do not add BaseStringHelper::contains().
2. in Arrayhelper::map() use strpos($to, '.') === false do detect if key contains dot.

If we really want some kind contains() helper in StringHelper, then IMO it should provide something that str_contains() do not provide. For example proper multibyte support - in that case it should consistently use mb_*functions and should not fallback to str_contains(). But this is out of scope of this PR.

[chriscpty]

alright, done :)

[samdark]

Thank you!