odan / phinx-migrations-generator

A Migration Code Generator for Phinx
https://odan.github.io/phinx-migrations-generator/
MIT License
231 stars 46 forks source link

Inconsistency after the migration process #98

Closed TiredFingers closed 3 years ago

TiredFingers commented 3 years ago

https://github.com/odan/phinx-migrations-generator/blob/81eca42a54dae99d5b10c4a16f5fd1254daf9be8/src/Migration/Adapter/Generator/PhinxMySqlColumnOptionGenerator.php#L203

When I use tinyint column type, I can set limit less than 4. Maybe we should use something like

min(INT_TINY, $column_data['LIMIT'])

instead of just mapping types?

odan commented 3 years ago

The term "limit" is a bit confusing in this context. See here

In Phinx the "limit" option defines a more specific integer type when the main type is "integer".

Example:

->addColumn('product_id', 'integer', ['limit' => MysqlAdapter::INT_BIG])
odan commented 3 years ago

I just fixed a Bug in combination with bigint(20). There was a incorrect mapping.

odan commented 3 years ago

The type tinyint(1) and other lengths like tinyint(4) are already supported. See this test

Please provide an example with (the MySQL version) if this is not working.

TiredFingers commented 3 years ago

Thanks for the answer I think I put it wrong. When I working with mysql, in phpmyadmin when adding new column, I can choose tinyint data type and set limit to 2. But when I tried to generate schema based on this field, I have tinyint(4) instead of 2

mysql 5.7.21 in docker windows 10x64 php 7.3

odan commented 3 years ago

This could also be an issue with phinx. Can you post a code snipped from the generated php code? (just the code from the affected column).

TiredFingers commented 3 years ago

Print screen of table structure: https://1drv.ms/u/s!AkGt2kBeBd6yguBQzQASZVC1w4SiyA?e=AHbS0D

->addColumn('status', 'integer', [ 'null' => true, 'default' => '0', 'limit' => MysqlAdapter::INT_TINY, 'comment' => 'Статус', 'after' => 'briefing', ])

ps - sure, it can be issue with phinx. I wanted you to understand me properly - I don't want to critics your work, I think you made great tool, thanks to you. Think of it as nothing more than a dialogue about possible improvements. I can do it by myself, but I want to hear your professional opinion, maybe there are some reasons that led to the current functionality or something else

odan commented 3 years ago

I just checked the Phinx documentation and other sources in the web. As far as I know this is not supported by Phinx. See here. As long this is not supported, the generator cannot create such options. Do you have a working Phinx example for tinyint(4)?

TiredFingers commented 3 years ago

Yes I have

odan commented 3 years ago

Can you post the working code snippet?

TiredFingers commented 3 years ago

If I rightly undestand your question - you need working code snippet which will create field with type tinyint(4). If it's correct - it's from above

->addColumn('status', 'integer', [ 'null' => true, 'default' => '0', 'limit' => MysqlAdapter::INT_TINY, 'comment' => 'Статус', 'after' => 'briefing', ])

or you want to see something defferent?

odan commented 3 years ago

Ok, thanks, I'll take a look at that.

odan commented 3 years ago

I think this question is still a little but unclear.

Example 1:

 ->addColumn('value_tinyint_1', 'boolean', [
                'null' => true,
                'default' => '0',
                'limit' => MysqlAdapter::INT_TINY,
            ])

Output:

`value_tinyint_1` tinyint(1) DEFAULT '0',

Example 2:

->addColumn('value_tinyint_4', 'integer', [
    'null' => true,
    'default' => '0',
    'limit' => MysqlAdapter::INT_TINY,
])

Generated sql

 `value_tinyint_4` tinyint(4) DEFAULT '0',

In phinx the default length/limit for tinyint is 4.

When I working with mysql, in phpmyadmin when adding new column, I can choose tinyint data type and set limit to 2. But when I tried to generate schema based on this field, I have tinyint(4) instead of 2

You try to get an SQL column definition like tinyint(2), correct?

As far as I know it's not possible / supported by Phinx to define other tinyint lengths like 1 and 4.

Phinx only supports tinyint(1) (alias "boolean") and tinyint(4).

That why I ask to provide an example Phinx code that can generate the desired output like tinyint(2).

TiredFingers commented 3 years ago

my mistake. I don't have such example, I don't understanded you properly, sorry. You right, phinx don't support such behavior. Do you think I need open same issue in phinx?

odan commented 3 years ago

Yes, I think this is not supported by Phinx. You may try to create an issue here and ask how to declare tinyint(2).

TiredFingers commented 3 years ago

Ok, thanks