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.9k forks source link

Abstract type is incorrect for primary keys while loading column schema #20209

Open SOHELAHMED7 opened 4 months ago

SOHELAHMED7 commented 4 months ago

What steps will reproduce the problem?

Though this issue is present in most of the databases supported by Yii, I am giving example of MySQL here.

Create a table:

        Yii::$app->db->createCommand()->createTable('{{%bigpks}}', [
            'id' => 'bigpk',
            'name' => 'string(150)',
        ])->execute();

What is the expected result?

\yii\db\ColumnSchema::$type for id column must be \yii\db\Schema::TYPE_BIGPK

Note: Loaded column schema can be obtained as Yii::$app->db->getTableSchema('{{%bigpks}}', true)->columns

What do you get instead?

It is \yii\db\Schema::TYPE_BIGINT.

Proposed solution

2024-06-22_19-42

Additional info

Q A
Yii version 2.0.?
PHP version
Operating system
bizley commented 4 months ago

I'm not sure if this was ever promised to keep the same column type between generating and fetching them. After all the configuration can be set as a generic one that will be interpreted by the DB engine, like with MySQL's boolean generated as tinyint(1). What is the problem here exactly that you are trying to solve?

SOHELAHMED7 commented 4 months ago

I'm not sure if this was ever promised to keep the same column type between generating and fetching them.

Agree but the ideal behaviour should be: keep same column type while generating and fetching them

What is the problem here exactly that you are trying to solve?

Please see https://github.com/cebe/yii2-openapi/issues/132 and its PR https://github.com/SOHELAHMED7/yii2-openapi/pull/29. If a component schema is removed from OpenAPI spec then its drop table migrations should be automatically generated. For more concrete example see https://github.com/SOHELAHMED7/yii2-openapi/pull/29/files#diff-766ce3ce55a7c3b75a01e734cf33eee6485bb9e3f1d402fdebab371df210dcfeR278-R352

and the generated migrated files are:

image

Note that currently it is working as expected because of the work-around I applied

bizley commented 4 months ago

Ok, this is easy for the primary keys since there is property in the schema that can be used. What about my example?

We are adding boolean column in MySQL. It comes back as tinyint. But we should have it as boolean to make it behave the same.

SOHELAHMED7 commented 4 months ago

This issue is for primary keys only.

But as far as tinyint(1)/bool is considered, they are synonym.

So for

Yii::$app->db->createCommand()->createTable('{{%bools}}', [
            'id' => 'pk',
            'boolv' => 'bool',
        ])->execute();

the generated code is

public function down()
    {
        $this->createTable('{{%bools}}', [
            'id' => $this->primaryKey(),
            'boolv' => $this->tinyInteger(1)->null()->defaultValue(null),
        ]);
    }

then it is completely fine and I don't see any issue in loading of column schema.

For primary keys, if they are not generated as PK, then I have to add separate SQL statement (CREATE PRIMARY KEY...) in migrations

rob006 commented 4 months ago

For primary keys, if they are not generated as PK, then I have to add separate SQL statement (CREATE PRIMARY KEY...) in migrations

You already need separate statements for foreign keys and other indexes. Doing the same for primary keys sounds like simple and consistent solution.

SOHELAHMED7 commented 4 months ago

Of course that will fix my issue(the work-around I applied) but this issue will still stand I believe.

rob006 commented 4 months ago

It would stand anyway for other cases, since we won't be able to reverse other aliases like boolean. pk is just an alias for int(11) NOT NULL AUTO_INCREMENT PRIMARY KEY, I'm not sure if reversing aliases should be the goal here.