Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove unnecessary isset() checks and fix key exists checks #19914

Merged
merged 8 commits into from
Aug 18, 2023
1 change: 1 addition & 0 deletions framework/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ Yii Framework 2 Change Log
- Enh #19884: Added support Enums in Query Builder (sk1t0n)
- Bug #19908: Fix associative array cell content rendering in Table widget (rhertogh)
- Bug #19906: Fixed multiline strings in the `\yii\console\widgets\Table` widget (rhertogh)
- Bug #19914: Fixed `ArrayHelper::keyExists()` and `::remove()` functions when the key is a float and the value is `null` (rhertogh)
- Enh #19920: Broadened the accepted type of `Cookie::$expire` from `int` to `int|string|\DateTimeInterface|null` (rhertogh)


Expand Down
4 changes: 2 additions & 2 deletions framework/db/BaseActiveRecord.php
Original file line number Diff line number Diff line change
Expand Up @@ -282,15 +282,15 @@ public function canSetProperty($name, $checkVars = true, $checkBehaviors = true)
*/
public function __get($name)
{
if (isset($this->_attributes[$name]) || array_key_exists($name, $this->_attributes)) {
if (array_key_exists($name, $this->_attributes)) {
return $this->_attributes[$name];
}

if ($this->hasAttribute($name)) {
return null;
}

if (isset($this->_related[$name]) || array_key_exists($name, $this->_related)) {
if (array_key_exists($name, $this->_related)) {
return $this->_related[$name];
}
$value = parent::__get($name);
Expand Down
18 changes: 13 additions & 5 deletions framework/helpers/BaseArrayHelper.php
Original file line number Diff line number Diff line change
Expand Up @@ -327,7 +327,12 @@ public static function setValue(&$array, $path, $value)
*/
public static function remove(&$array, $key, $default = null)
{
if (is_array($array) && (isset($array[$key]) || array_key_exists($key, $array))) {
// ToDo: This check can be removed when the minimum PHP version is >= 8.1 (Yii2.2)
if (is_float($key)) {
$key = (int)$key;
}

if (is_array($array) && array_key_exists($key, $array)) {
$value = $array[$key];
unset($array[$key]);

Expand Down Expand Up @@ -608,17 +613,20 @@ public static function map($array, $from, $to, $group = null)
* Checks if the given array contains the specified key.
* This method enhances the `array_key_exists()` function by supporting case-insensitive
* key comparison.
* @param string $key the key to check
* @param string|int $key the key to check
* @param array|ArrayAccess $array the array with keys to check
* @param bool $caseSensitive whether the key comparison should be case-sensitive
* @return bool whether the array contains the specified key
*/
public static function keyExists($key, $array, $caseSensitive = true)
{
// ToDo: This check can be removed when the minimum PHP version is >= 8.1 (Yii2.2)
if (is_float($key)) {
$key = (int)$key;
}

if ($caseSensitive) {
// Function `isset` checks key faster but skips `null`, `array_key_exists` handles this case
// https://www.php.net/manual/en/function.array-key-exists.php#107786
if (is_array($array) && (isset($array[$key]) || array_key_exists($key, $array))) {
if (is_array($array) && array_key_exists($key, $array)) {
return true;
}
// Cannot use `array_has_key` on Objects for PHP 7.4+, therefore we need to check using [[ArrayAccess::offsetExists()]]
Expand Down
60 changes: 56 additions & 4 deletions tests/framework/helpers/ArrayHelperTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,29 @@ public function testRemove()
$this->assertEquals('defaultValue', $default);
}

/**
* @return void
*/
public function testRemoveWithFloat()
{
if (version_compare(PHP_VERSION, '8.1.0', '>=')) {
$this->markTestSkipped('Using floats as array key is deprecated.');
}

$array = ['name' => 'b', 'age' => 3, 1.1 => null];

$name = ArrayHelper::remove($array, 'name');
$this->assertEquals($name, 'b');
$this->assertEquals($array, ['age' => 3, 1.1 => null]);

$floatVal = ArrayHelper::remove($array, 1.1);
$this->assertNull($floatVal);
$this->assertEquals($array, ['age' => 3]);

$default = ArrayHelper::remove($array, 'nonExisting', 'defaultValue');
$this->assertEquals('defaultValue', $default);
}

public function testRemoveValueMultiple()
{
$array = [
Expand Down Expand Up @@ -506,14 +529,21 @@ public function testMergeEmpty()
/**
* @see https://github.com/yiisoft/yii2/pull/11549
*/
public function test()
public function testGetValueWithFloatKeys()
{
$array = [];
$array[1.0] = 'some value';
if (version_compare(PHP_VERSION, '8.1.0', '>=')) {
$this->markTestSkipped('Using floats as array key is deprecated.');
}

$result = ArrayHelper::getValue($array, 1.0);
$array = [];
$array[1.1] = 'some value';
$array[2.1] = null;

$result = ArrayHelper::getValue($array, 1.2);
$this->assertEquals('some value', $result);

$result = ArrayHelper::getValue($array, 2.2);
$this->assertNull($result);
}

public function testIndex()
Expand Down Expand Up @@ -712,6 +742,7 @@ public function testKeyExists()
'a' => 1,
'B' => 2,
];

$this->assertTrue(ArrayHelper::keyExists('a', $array));
$this->assertFalse(ArrayHelper::keyExists('b', $array));
$this->assertTrue(ArrayHelper::keyExists('B', $array));
Expand All @@ -723,6 +754,27 @@ public function testKeyExists()
$this->assertFalse(ArrayHelper::keyExists('c', $array, false));
}

public function testKeyExistsWithFloat()
{
if (version_compare(PHP_VERSION, '8.1.0', '>=')) {
$this->markTestSkipped('Using floats as array key is deprecated.');
}

$array = [
1 => 3,
2.2 => 4, // Note: Floats are cast to ints, which means that the fractional part will be truncated.
3.3 => null,
];

$this->assertTrue(ArrayHelper::keyExists(1, $array));
$this->assertTrue(ArrayHelper::keyExists(1.1, $array));
$this->assertTrue(ArrayHelper::keyExists(2, $array));
$this->assertTrue(ArrayHelper::keyExists('2', $array));
$this->assertTrue(ArrayHelper::keyExists(2.2, $array));
$this->assertTrue(ArrayHelper::keyExists(3, $array));
$this->assertTrue(ArrayHelper::keyExists(3.3, $array));
}

public function testKeyExistsArrayAccess()
{
$array = new TraversableArrayAccessibleObject([
Expand Down
Loading