Remove unnecessary isset() checks and fix key exists checks

[rhertogh]

Q	A
Is bugfix?	✔️
New feature?	❌
Is enhancement	✔️
Breaks BC?	❌
Fixed issues	-

Until PHP version 7.3 the array_key_exists() function was relatively slow. A workaround for it was to first do an isset() check.
Since PHP 7.4 the array_key_exists() is faster than isset(), let alone doing both checks.

This PR removes the now unnecessary isset() checks and fixes a bug where floats are passed as key.
Originally described in #11549 PHP casts float keys to integer, however, the isset() function will return false in case the key's value is null resulting in a float being passed to array_key_exists() which causes the exception "array_key_exists(): The first argument should be either a string or an integer".
This is solved by casting the $key to int in case it's a float.

[what-the-diff]

PR Summary

• Improved Code Robustness in framework/db/BaseActiveRecord.php
  The method of checking for the existence of certain key elements in the array in BaseActiveRecord.php was streamlined. We switched from an inconsistent use of isset or array_key_exists to solely using array_key_exists. This will ensure a more reliable and consistent way to verify the presence of keys in the array.

• Enhanced Consistency in framework/helpers/BaseArrayHelper.php
  The consistency of array-related operations was also improved in the BaseArrayHelper.php file. We've shifted fully to array_key_exists for checking if a key is present in an array in the setValue and keyExists methods. This modification enhances the reliability and uniformity of our code.

[codecov]

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.01% ⚠️

Comparison is base (73902f0) 48.90% compared to head (f1cb414) 48.89%.
Report is 3 commits behind head on master.

❗ Current head f1cb414 differs from pull request most recent head e847781. Consider uploading reports for the commit e847781 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #19914      +/-   ##
==========================================
- Coverage   48.90%   48.89%   -0.01%     
==========================================
  Files         445      445              
  Lines       42806    42790      -16     
==========================================
- Hits        20935    20923      -12     
+ Misses      21871    21867       -4     

Files Changed	Coverage Δ
framework/db/BaseActiveRecord.php	74.06% <100.00%> (ø)
framework/helpers/BaseArrayHelper.php	100.00% <100.00%> (ø)

... and 4 files with indirect coverage changes

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

[PowerGamer1]

Shouldn't you first remove SUPPORT for PHP5 before removing optimizations for it? Unless it goes into Yii 2.2 but it does not say so here anywhere.

[rob006]

This code still works for PHP <7.3, just slightly slower, so this is not really a BC break. According to https://packagist.org/packages/yiisoft/yii2/php-stats these are less than 10% of installs, so improving performance for 90% of users at the cost of decreasing performance 10% users, still makes sense (especially that if you care about performance, you would not stuck of PHP 7.2 or lower).

[PowerGamer1]

This code still works for PHP <7.3, just slightly slower, so this is not really a BC break.

It's not, but it should be (see below).

According to https://packagist.org/packages/yiisoft/yii2/php-stats these are less than 10% of installs, so improving performance for 90% of users at the cost of decreasing performance 10% users, still makes sense (especially that if you care about performance, you would not stuck of PHP 7.2 or lower).

Except you never measured the amount of performance improvement for 10% of users (ain't you were quick to dismiss MY performance claims as "negligible" only yesterday in #19788?) but you surely decreased the performance notably (if it wasn't notable those optimizations wouldn't exist in the first place) for the 10% of users you consider to be SUPPORTED.

If you don't want to drop PHP5 support in Yii2 you must put this PR change in Yii2.2 because it is detrimental for users on PHP5 you still claim to support and the users of PHP7+ will have no problem with removed support for PHP5 and so will be able to switch to Yii2.2 without problems. This way both 90% AND 10% of supported users win.

[rob006]

This was always a micro-optimization and thus it is hard to benchmark this (if I have for loop with isset() call, only half of the execution time is taken by isset(), the rest comes from the loop). It is also hard to measure real impact, because it depends on the case - current implementation is faster for old PHP when you're accessing set attribute, but it is always slower (even on older PHP) in any other case (accessing relation, virtual attribute, unset attribute etc.), because you have both isset() and array_key_exists() calls that return false. And to be clear, we're talking about "function call overhead" here, this may be noticeable only in some tight loops when attributes of the same model are accessed over and over.

I think that we should not aim with such micro-optimization to old and unsupported versions of PHP, especially if it decreases performance of recent versions used by 90% of users. That is actually a micro-deoptimization. :P

[PowerGamer1]

This was always a micro-optimization and thus it is hard to benchmark this (if I have for loop with isset() call, only half of the execution time is taken by isset(), the rest comes from the loop). It is also hard to measure real impact, because it depends on the case - current implementation is faster for old PHP when you're accessing set attribute, but it is always slower (even on older PHP) in any other case (accessing relation, virtual attribute, unset attribute etc.), because you have both isset() and array_key_exists() calls that return false. And to be clear, we're talking about "function call overhead" here, this may be noticeable only in some tight loops when attributes of the same model are accessed over and over.

I think that we should not aim with such micro-optimization to old and unsupported versions of PHP, especially if it decreases performance of recent versions used by 90% of users. That is actually a micro-deoptimization. :P

@rob006
Sorry to sound rude, but I am done wasting my time on pointless arguing with you specifically. If the rest of the team cares about the engine at all they will take my concerns into consideration and act accordingly.

[mtangoo]

but I am done wasting my time on pointless arguing with you specifically
I do not see any of you wasting any of his time with useful point that both of you are rising. Just you have to agree that sometimes you can happen to see things with different lens. And that is okay as long as it doesn't amount to fight or something.

For example in this you are concerned of the performance degradation of old PHP version. Your option is "Support it like the rest supported or drop support altogether". Rob have the same view but only he thinks that someone who clings to old 5.6 should have some weight on his own shoulder, as practically there is no any serious reason not to upgrade to 7.x (correct me if am wrong)

In such discussion both are useful argument and the question really is, how much do we want support parity to be in those old PHP versions.

My opinion is we should really ignore some issues like this, with big note on upgrade notes/change log that if you are using PHP5.6 for example, above change will affect you this way. And suggest upgrade to 7.x

Subtle deprecation of the framework support for 5.6 if you wish before actual support is dropped. That is one example of approach we can take. But honestly rather than you guys debate two valid use cases viewed differently, I would suggest you propose couple of solutions. Then it will be easier to discuss and pick one of those solutions.

So what do you propose we do with this @PowerGamer1? I see Rob have proposed this one loud and clear

I think that we should not aim with such micro-optimization to old and unsupported versions of PHP, especially if it decreases performance of recent versions used by 90% of users. That is actually a micro-deoptimization

[rhertogh]

... If you don't want to drop PHP5 support in Yii2 ...

Nowhere in this PR here is anything that "drops" support for PHP v5. It actually increases compatibility for all version < 8.1.
As @rob006 pointed out: The only thing that is changing is the focus of optimization, which is currently at 10% of the users and is shifted to to other 90%.

[PowerGamer1]

So what do you propose we do with this @PowerGamer1?

If you want to keep your claim that you still SUPPORT PHP5 then have a decency to do such support properly or at least NOT remove ALREADY EXISTING optimizations for PHP5. Otherwise get rid of that support ENTIRELY and then feel free to improve the code without being limited by obligations and constraints the claim of "SUPPORT" comes with.

My personal preference is dropping support for PHP5. Either you do it in Yii2 or Yii2.2 is up to you. But that commit should only go into the branch without PHP5 support.

Nowhere in this PR here is anything that "drops" support for PHP v5.

That is exactly the problem - you are not dropping it, but you should.

The only thing that is changing is the focus of optimization, which is currently at 10% of the users and is shifted to to other 90%.

Instead of making optimizations for 100% of user base by dropping support for PHP5 which is more than very long overdue.

P.S. Please do whatever you want and don't reply to me in this topic any longer. You have your opinion and I have mine, lets leave it at that.

[samdark]

Thanks.