Skip to content

Remove flags from getPrimaryKey() and getOldPrimaryKey() methods #422

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
May 18, 2025
Merged

Remove flags from getPrimaryKey() and getOldPrimaryKey() methods #422

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c671982
Remove flags from `getPrimaryKey()` and `getOldPrimaryKey()` methods
Tigrov May 17, 2025
c9df1e7
Throw Exception if no primary keys
Tigrov May 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 46 additions & 19 deletions src/AbstractActiveRecord.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ public function equals(ActiveRecordInterface $record): bool
return false;
}

return $this->getTableName() === $record->getTableName() && $this->getPrimaryKey() === $record->getPrimaryKey();
return $this->getTableName() === $record->getTableName() && $this->getPrimaryKeys() === $record->getPrimaryKeys();
}

public function get(string $propertyName): mixed
Expand Down Expand Up @@ -180,25 +180,33 @@ public function oldValues(): array
return $this->oldValues ?? [];
}

/**
* @throws InvalidConfigException
* @throws Exception
*/
public function getOldPrimaryKey(bool $asArray = false): mixed
public function getOldPrimaryKey(): float|int|string|null
{
$keys = $this->primaryKey();

return match (count($keys)) {
1 => $this->oldValues[$keys[0]] ?? null,
0 => throw new Exception(
static::class . ' does not have a primary key. You should either define a primary key for '
. $this->getTableName() . ' table or override the primaryKey() method.'
),
default => throw new Exception(
static::class . ' has multiple primary keys. Use getOldPrimaryKeys() method instead.'
),
};
}

public function getOldPrimaryKeys(): array
{
$keys = $this->primaryKey();

if (empty($keys)) {
throw new Exception(
static::class . ' does not have a primary key. You should either define a primary key for '
. 'the corresponding table or override the primaryKey() method.'
. $this->getTableName() . ' table or override the primaryKey() method.'
);
}

if ($asArray === false && count($keys) === 1) {
return $this->oldValues[$keys[0]] ?? null;
}

$values = [];

foreach ($keys as $name) {
Expand All @@ -208,12 +216,31 @@ public function getOldPrimaryKey(bool $asArray = false): mixed
return $values;
}

public function getPrimaryKey(bool $asArray = false): mixed
public function getPrimaryKey(): float|int|string|null
{
$keys = $this->primaryKey();

if ($asArray === false && count($keys) === 1) {
return $this->get($keys[0]);
return match (count($keys)) {
1 => $this->get($keys[0]),
0 => throw new Exception(
static::class . ' does not have a primary key. You should either define a primary key for '
. $this->getTableName() . ' table or override the primaryKey() method.'
),
default => throw new Exception(
static::class . ' has multiple primary keys. Use getPrimaryKeys() method instead.'
),
};
}

public function getPrimaryKeys(): array
{
$keys = $this->primaryKey();

if (empty($keys)) {
throw new Exception(
static::class . ' does not have a primary key. You should either define a primary key for '
. $this->getTableName() . ' table or override the primaryKey() method.'
);
}

$values = [];
Expand Down Expand Up @@ -554,7 +581,7 @@ public function populateRelation(string $name, array|ActiveRecordInterface|null
*/
public function refresh(): bool
{
$record = $this->query()->findByPk($this->getPrimaryKey(true));
$record = $this->query()->findByPk($this->getOldPrimaryKeys());

return $this->refreshInternal($record);
}
Expand Down Expand Up @@ -758,7 +785,7 @@ public function updateAllCounters(array $counters, array|string $condition = '',
*/
public function updateCounters(array $counters): bool
{
if ($this->updateAllCounters($counters, $this->getOldPrimaryKey(true)) === 0) {
if ($this->updateAllCounters($counters, $this->getOldPrimaryKeys()) === 0) {
return false;
}

Expand Down Expand Up @@ -864,7 +891,7 @@ public function unlink(string $relationName, ActiveRecordInterface $arClass, boo
/** @psalm-var array<array-key, ActiveRecordInterface> $related */
$related = $this->related[$relationName];
foreach ($related as $a => $b) {
if ($arClass->getPrimaryKey() === $b->getPrimaryKey()) {
if ($arClass->getPrimaryKeys() === $b->getPrimaryKeys()) {
unset($this->related[$relationName][$a]);
}
}
Expand Down Expand Up @@ -1044,7 +1071,7 @@ protected function deleteInternal(): int
* We don't check the return value of deleteAll() because it is possible the record is already deleted in
* the database and thus the method will return 0
*/
$condition = $this->getOldPrimaryKey(true);
$condition = $this->getOldPrimaryKeys();

if ($this instanceof OptimisticLockInterface) {
$lock = $this->optimisticLockPropertyName();
Expand Down Expand Up @@ -1130,7 +1157,7 @@ protected function updateInternal(array|null $properties = null): int
return 0;
}

$condition = $this->getOldPrimaryKey(true);
$condition = $this->getOldPrimaryKeys();

if ($this instanceof OptimisticLockInterface) {
$lock = $this->optimisticLockPropertyName();
Expand Down
59 changes: 30 additions & 29 deletions src/ActiveRecordInterface.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,47 +127,48 @@ public function propertyValues(array|null $names = null, array $except = []): ar
public function getIsNewRecord(): bool;

/**
* Returns the old primary key value(s).
* Returns the old value of the primary key as a scalar.
*
* This refers to the primary key value that's populated into the record after executing a find method (for example,
* `findOne()`).
* This refers to the primary key value that is populated into the record after data is retrieved from the database
* by an instance of {@see ActiveQueryInterface}.
*
* The value remains unchanged even if the primary key property is manually assigned with a different value.
* The value remains unchanged while the record will not be {@see update() updated}.
*
* @param bool $asArray Whether to return the primary key value as an array. If true, the return value will be an
* array with property name as key and property value as value. If this is `false` (default), a scalar value will be
* returned for a non-composite primary key.
* @throw Exception If multiple primary keys or no primary key.
*
* @return mixed The old primary key value. An array (property name => property value) is returned if the primary
* key is composite or `$asArray` is true. A string is returned otherwise (`null` will be returned if the key value
* is `null`).
* @see getOldPrimaryKeys()
*/
public function getOldPrimaryKey(): float|int|string|null;

/**
* Returns the old values of the primary key as an array with property names as keys and property values as values.
*
* This refers to the primary key values that is populated into the record after data is retrieved from the database
* by an instance of {@see ActiveQueryInterface}.
*
* The value remains unchanged while the record will not be {@see update() updated}.
*
* @psalm-return (
* $asArray is true
* ? array<string, mixed>
* : mixed|null
* )
* @throw Exception If no primary key or multiple primary keys.
*
* @see getOldPrimaryKeys()
*/
public function getOldPrimaryKey(bool $asArray = false): mixed;
public function getOldPrimaryKeys(): array;

/**
* Returns the primary key value(s).
* Returns the scalar value of the primary key.
*
* @param bool $asArray Whether to return the primary key value as an array. If true, the return value will be an
* array with property names as keys and property values as values. Note that for composite primary keys, an array
* will always be returned regardless of this parameter value.
* @throw Exception If multiple primary keys or no primary key.
*
* @return mixed The primary key value. An array (property name => property value) is returned if the primary key
* is composite or `$asArray` is true. A string is returned otherwise (`null` will be returned if the key value is
* `null`).
* @see getPrimaryKeys()
*/
public function getPrimaryKey(): float|int|string|null;

/**
* Returns the values of the primary key as an array with property names as keys and property values as values.
*
* @psalm-return (
* $asArray is true
* ? array<string, mixed>
* : mixed|null
* )
* @see getPrimaryKey()
*/
public function getPrimaryKey(bool $asArray = false): mixed;
public function getPrimaryKeys(): array;

/**
* Return the name of the table associated with this AR class.
Expand Down
80 changes: 67 additions & 13 deletions tests/ActiveRecordTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -634,34 +634,88 @@ public function testJoinWithEager(): void

public function testSaveWithoutChanges(): void
{
$this->reloadFixtureAfterTest();

$customerQuery = new ActiveQuery(Customer::class);

$customer = $customerQuery->findByPk(1);
$customer = Customer::findByPk(1);

$this->assertTrue($customer->save());
}

public function testGetPrimaryKey(): void
{
$customerQuery = new ActiveQuery(Customer::class);

$customer = $customerQuery->findByPk(1);
$customer = Customer::findByPk(1);

$this->assertSame(1, $customer->getPrimaryKey());
$this->assertSame(['id' => 1], $customer->getPrimaryKey(true));
$this->assertSame(['id' => 1], $customer->getPrimaryKeys());

$orderItemQuery = new ActiveQuery(OrderItem::class);
$orderItem = $orderItemQuery->findByPk([1, 2]);

$this->assertSame(['order_id' => 1, 'item_id' => 2], $orderItem->getPrimaryKeys());

$this->expectException(Exception::class);
$this->expectExceptionMessage(OrderItem::class . ' has multiple primary keys. Use getPrimaryKeys() method instead.');

$orderItem->getPrimaryKey();
}

public function testGetOldPrimaryKey(): void
public function testGetPrimaryKeyWithoutPrimaryKey(): void
{
$customerQuery = new ActiveQuery(Customer::class);
$orderItem = new OrderItemWithNullFK();

$customer = $customerQuery->findByPk(1);
$this->expectException(Exception::class);
$this->expectExceptionMessage(OrderItemWithNullFK::class . ' does not have a primary key.');

$orderItem->getPrimaryKey();
}

public function testGetPrimaryKeysWithoutPrimaryKey(): void
{
$orderItem = new OrderItemWithNullFK();

$this->expectException(Exception::class);
$this->expectExceptionMessage(OrderItemWithNullFK::class . ' does not have a primary key.');

$orderItem->getPrimaryKeys();
}

public function testGetOldPrimaryKey(): void
{
$customer = Customer::findByPk(1);
$customer->setId(2);

$this->assertSame(1, $customer->getOldPrimaryKey());
$this->assertSame(['id' => 1], $customer->getOldPrimaryKey(true));
$this->assertSame(['id' => 1], $customer->getOldPrimaryKeys());

$orderItemQuery = new ActiveQuery(OrderItem::class);
$orderItem = $orderItemQuery->findByPk([1, 2]);
$orderItem->setOrderId(3);
$orderItem->setItemId(4);

$this->assertSame(['order_id' => 1, 'item_id' => 2], $orderItem->getOldPrimaryKeys());

$this->expectException(Exception::class);
$this->expectExceptionMessage(OrderItem::class . ' has multiple primary keys. Use getOldPrimaryKeys() method instead.');

$orderItem->getOldPrimaryKey();
}

public function testGetOldPrimaryKeyWithoutPrimaryKey(): void
{
$orderItem = new OrderItemWithNullFK();

$this->expectException(Exception::class);
$this->expectExceptionMessage(OrderItemWithNullFK::class . ' does not have a primary key.');

$orderItem->getOldPrimaryKey();
}

public function testGetOldPrimaryKeysWithoutPrimaryKey(): void
{
$orderItem = new OrderItemWithNullFK();

$this->expectException(Exception::class);
$this->expectExceptionMessage(OrderItemWithNullFK::class . ' does not have a primary key.');

$orderItem->getOldPrimaryKeys();
}

public function testGetDirtyValuesOnNewRecord(): void
Expand Down
Loading