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

on update current_timestamp() seems to be lost in migration #115

Closed
samuelgfeller opened this issue Apr 11, 2024 · 4 comments · Fixed by #116
Closed

on update current_timestamp() seems to be lost in migration #115

samuelgfeller opened this issue Apr 11, 2024 · 4 comments · Fixed by #116
Labels

Comments

@samuelgfeller
Copy link
Contributor

samuelgfeller commented Apr 11, 2024

Hi Daniel

I created a table with the column updated_at which should hold the current_timestamp() value when the row is updated.

When running phinx-migrations generate --overwrite -c config/env/env.phinx.php --ansi and look at the generated schema.php I see the following:

'tables' => 
  array (
    'user' => 
    array (
      'table' => 
      array (
        'table_name' => 'user',
        'engine' => 'InnoDB',
        'table_comment' => '',
        'table_collation' => 'utf8mb4_general_ci',
        'character_set_name' => 'utf8mb4',
        'row_format' => 'Dynamic',
      ),
      'columns' => 
      array (
        // ...
        'updated_at' => 
        array (
          // ...
          'COLUMN_NAME' => 'updated_at',
          // ...
          'EXTRA' => 'on update current_timestamp()',
// ...

So it has been picked up by the generation with the "EXTRA" key but if I now delete the user table in the database and the phinx log table and run composer migrate or vendor/bin/phinx migrate -c config/env/env.phinx.php --ansi -vvv the table is generated without the ON UPDATE value, it's just blank.

Is that a phinx bug or is something wrong with the generated schema.php?

Thanks for looking into this!

Edit:

I'm using the latest version 6.1.0.

DDL before phinx-migrations generate

create table user
(
    id  int auto_increment primary key,
    updated_at datetime null on update current_timestamp()
) 
row_format = DYNAMIC;

Migration creation log in the console when running the phinx migrate command after deleting the user and phinx log table

CREATE TABLE `user` (`id` INT(11) NOT NULL AUTO_INCREMENT, `updated_at` DATETIME NULL, PRIMARY KEY (`id`)) ENGINE = InnoDB CHARACTER SET utf8mb4 COLLATE utf8mb4_general_ci COMMENT=''  ROW_FORMAT=DYNAMIC;

DDL after :

create or replace table user
(
    id int auto_increment primary key,
    updated_at datetime null
)
row_format = DYNAMIC;
@odan
Copy link
Owner

odan commented Apr 11, 2024

Can you show the generated migration class?

@samuelgfeller
Copy link
Contributor Author

Sure!

<?php

use Phinx\Db\Adapter\MysqlAdapter;

class DbChange10364213506617e41c8631a extends Phinx\Migration\AbstractMigration
{
    public function change()
    {
        $this->table('user', [
                'id' => false,
                'primary_key' => ['id'],
                'engine' => 'InnoDB',
                'encoding' => 'utf8mb4',
                'collation' => 'utf8mb4_general_ci',
                'comment' => '',
                'row_format' => 'DYNAMIC',
            ])
            ->addColumn('updated_at', 'datetime', [
                'null' => true,
                'default' => null,
                'after' => 'email',
            ])
            ->create();
    }
}

@odan
Copy link
Owner

odan commented Apr 11, 2024

It looks like a bug in the class PhinxMySqlColumnOptionGenerator.

See here:

if (strpos($columnData['EXTRA'], 'on update CURRENT_TIMESTAMP') !== false) {

The strpos is case-sensitive. The stripos function is case-insensitive and should fix that issue.

Demo: https://3v4l.org/XXUg6

Can you create a PR?

@samuelgfeller
Copy link
Contributor Author

Well done finding this bug! I've made a PR making the statement case-insensitive.

@odan odan closed this as completed in #116 Apr 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants