yiisoft / yii2

Yii 2: The Fast, Secure and Professional PHP Framework
http://www.yiiframework.com
BSD 3-Clause "New" or "Revised" License
14.24k stars 6.91k forks source link

Is BaseAcriveRecord::isPrimaryKey() works correct? #18108

Closed andrey-denisenko closed 4 years ago

andrey-denisenko commented 4 years ago

The problem:

I have ActiveRecord model with two primary keys (id and timestamp). So when I doing check for primary key: $model::isPrimaryKey('id') it returns me FALSE.

Because body of isPrimaryKey is following:

    public static function isPrimaryKey($keys)
    {
        $pks = static::primaryKey();
        if (count($keys) === count($pks)) {
            return count(array_intersect($keys, $pks)) === count($pks);
        }

        return false;
    }

So it does comparison amount of provided keys with amount of existing PK's. (what for???) This seems to be wrong, because 'id' is a primary key of my model, and obvious result shall be TRUE.

How it all happened?

I started to play with la-haute-societe/yii2-save-relations-behavior whose usage was proposed in this >>>article<<< This "helper behavior" does automation of saving of all related records.

So my model looks similar to this:

/**
 * Class MyModel
 *
 * columns:
 * @property int    $id << PK
 * @property int    $timestamp << PK
 * @property int    $type_id
 * @property string $details
 *
 * relations:
 * @property TypeModel $relatedType
 */
class MyModel extends ActiveRecord
{
    public function behaviors()
    {
        return [
            'saveRelations' => [
                'class' => SaveRelationsBehavior::class,
                'relations' => ['relatedType'],
            ]
        ];
    }

    public function transactions()
    {
        return [
            self::SCENARIO_DEFAULT => self::OP_ALL,
        ];
    }

    public function getRelatedType(): ActiveQuery
    {
        return $this->hasOne(TypeModel::class, ['id' => 'type_id']);
    }
}

And when I trying to save my model with relations, this helper behavior is failing on call $model::isPrimaryKey() Because MyModel has two primary keys, when SaveRelationsBehavior provides into isPrimaryKey() only 'id' (which was extracted from information about this relation).

Peace of failing code from SaveRelationsBehavior:

    private function _prepareHasOneRelation(BaseActiveRecord $model, $relationName, ModelEvent $event)
    {
        Yii::debug("_prepareHasOneRelation for {$relationName}", __METHOD__);
        $relationModel = $model->{$relationName};
        $this->validateRelationModel(self::prettyRelationName($relationName), $relationName, $model->{$relationName});
        $relation = $model->getRelation($relationName);
        $p1 = $model->isPrimaryKey(array_keys($relation->link)); <<<<<<<<< HERE only 'id' is passed
        $p2 = $relationModel::isPrimaryKey(array_values($relation->link));

        if ($relationModel->getIsNewRecord() && $p1 && !$p2) {
            // Save Has one relation new record
            if ($event->isValid && (count($model->dirtyAttributes) || $model->{$relationName}->isNewRecord)) {
                Yii::debug('Saving ' . self::prettyRelationName($relationName) . ' relation model', __METHOD__);
                if ($model->{$relationName}->save()) {
                    $this->_savedHasOneModels[] = $model->{$relationName};
                }
            }
        }
    }

So my question is:

It's the YII code is wrong? Or it's author of this SaveRelationsBehavior uses function isPrimaryKey() wrong way?

Q A
Yii version 2.0.3
PHP version ~7.2.2
Operating system Alpine Linux v3.11 (4.15.0-58-generic)
samdark commented 4 years ago

What do you mean by "model with two primary keys"? There could be only a single primary key. If you're using multiple columns, that's compound primary key i.e. both id and timestamp are part of the key so "id" by itself is not primary key in this case.

andrey-denisenko commented 4 years ago

What do you mean by "model with two primary keys"?

CREATE TABLE `MyModel` (
  `id` bigint(20) unsigned NOT NULL AUTO_INCREMENT,
  `timestamp_utc` int(10) unsigned NOT NULL,
  `type_id` int(11) NOT NULL,
  `details` blob,
  PRIMARY KEY (`id`,`timestamp_utc`),
)

So yes, right term will be "compound PK".

As I guess, mentioned extension shall use ActiveRecord::primaryKey() + in_array(), for example.

samdark commented 4 years ago

Then it works as expected. id is not primary key. timestamp_utc is not primary key. id,timestamp_utc is primary key.