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 @@ -14,6 +14,7 @@ Yii Framework 2 Change Log
- Bug #19868: Added whitespace sanitation for tests, due to updates in ICU 72 (schmunk42)
- Enh #19884: Added support Enums in Query Builder (sk1t0n)
- 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)


2.0.48.1 May 24, 2023
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
16 changes: 12 additions & 4 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))) {
// This check can be removed when the minimum PHP version is >= 8.1 (Yii2.2)
rhertogh marked this conversation as resolved.
Show resolved Hide resolved
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,16 +613,19 @@ 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)
{
// 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))) {
rhertogh marked this conversation as resolved.
Show resolved Hide resolved
return true;
}
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