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

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

Open
SOHELAHMED7 opened this issue Jun 22, 2024 · 7 comments
Open

Comments

@SOHELAHMED7
Copy link
Contributor

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
SOHELAHMED7 added a commit to SOHELAHMED7/yii2-openapi that referenced this issue Jun 22, 2024
@bizley
Copy link
Member

bizley commented Jun 23, 2024

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
Copy link
Contributor Author

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 cebe/yii2-openapi#132 and its PR SOHELAHMED7/yii2-openapi#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
Copy link
Member

bizley commented Jun 24, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor

rob006 commented Jun 24, 2024

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
Copy link
Contributor Author

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

@rob006
Copy link
Contributor

rob006 commented Jun 24, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants