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

Unnecessary migration commands on datetime fields #1121

Open
ppaulis opened this issue Feb 1, 2021 · 5 comments
Open

Unnecessary migration commands on datetime fields #1121

ppaulis opened this issue Feb 1, 2021 · 5 comments

Comments

@ppaulis
Copy link

ppaulis commented Feb 1, 2021

Bug Report

PHP 7.4.14
Doctrine Migrations 3.0.2
MySQL 5.7 and 8.0
Laminas API Tools application with doctrine/doctrine-orm-module 3.1.2
Docker-compose environment

Q A
BC Break no
Version 3.0.2

Summary

When generating a migration file with vendor/bin/doctrine-module migrations:diff, I always get this for my datetime fields:

$this->addSql('ALTER TABLE table_name CHANGE field_name field_name DATETIME NOT NULL');

despite the table definition already being like this:

CREATE TABLE `table_name` (
  ...
  `field_name` datetime NOT NULL,
  ...
  PRIMARY KEY (`id`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8 COLLATE=utf8_unicode_ci

Executing this migration of course doesn't do anything. And generating a new migration file afterwards has again the same result. It's also no metadata caching issue. Cache is not used in development environment.

Also the validate-schema command shows this:

root@6acb76fd3968:/var/www/html/api# vendor/bin/doctrine-module orm:validate-schema

Mapping
-------                                                                                                                 
 [OK] The mapping files are correct.                                                                                    
                                                                                                                        
Database
--------                                                                                                                    
 [ERROR] The database schema is not in sync with the current mapping file.                                              
                                                                                                                        
root@6acb76fd3968:/var/www/html/api#

My first guess would be a bug, but perhaps I'm doing simply the configuration wrong? Any help is welcome :-)

Expected behavior

Having an empty migration file if the current schema is in sync with the mapping files.

@nspyke
Copy link

nspyke commented Feb 28, 2021

I'm seeing the exact same behaviour with Doctrine ORM 2.8.2 and Migrations 3.1.0

final class Version20210228040259 extends AbstractMigration
{
    public function getDescription() : string
    {
        return '';
    }

    public function up(Schema $schema) : void
    {
        // this up() migration is auto-generated, please modify it to your needs
        $this->addSql('ALTER TABLE asset_usage_readings ALTER taken_at TYPE TIMESTAMP(0) WITH TIME ZONE');
        $this->addSql('ALTER TABLE asset_usage_readings ALTER taken_at DROP DEFAULT');
        $this->addSql('ALTER TABLE thermal_readings ALTER "timestamp" TYPE TIMESTAMP(0) WITH TIME ZONE');
        $this->addSql('ALTER TABLE thermal_readings ALTER "timestamp" DROP DEFAULT');
    }

    public function down(Schema $schema) : void
    {
        // this down() migration is auto-generated, please modify it to your needs
        $this->addSql('CREATE SCHEMA public');
        $this->addSql('ALTER TABLE asset_usage_readings ALTER taken_at TYPE TIMESTAMP(0) WITH TIME ZONE');
        $this->addSql('ALTER TABLE asset_usage_readings ALTER taken_at DROP DEFAULT');
        $this->addSql('ALTER TABLE thermal_readings ALTER timestamp TYPE TIMESTAMP(0) WITH TIME ZONE');
        $this->addSql('ALTER TABLE thermal_readings ALTER timestamp DROP DEFAULT');
    }
}
create table asset_usage_readings
(
    taken_at   timestamp(0) with time zone not null,
...
    constraint asset_usage_readings_pkey
        primary key (taken_at, vehicle_id)
);
create table thermal_readings
(
    timestamp      timestamp(0) with time zone not null,
...
    constraint thermal_readings_pkey
        primary key (timestamp, device_address)
);

Both these tables use the timestamp along with another column as the primary key due to Time Series partitioning requirements

@SenseException
Copy link
Member

Can you please provide a mapping example that results in that behaviour?

@ppaulis
Copy link
Author

ppaulis commented Mar 2, 2021

@SenseException just found the source of the problem. It's because I used datetimetz. I'm handling timezones in the application-layer in fact, but having worked on a Postgres-based application clearly didn't do any good here 🤦‍♂️

As for @nspyke 's comment, it seems to be Postgres and not MySQL. So I'll leave it up to you to close this issue or keep it open :-)

Thanks!

@nspyke
Copy link

nspyke commented Mar 2, 2021

Yes, I'm using Postgres, and I'm using datetimetz as a preference throughout my application.

Here's my mapping for the asset_usage_readings table

<?xml version="1.0" encoding="utf-8"?>
<doctrine-mapping xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
                  xmlns="http://doctrine-project.org/schemas/orm/doctrine-mapping"
                  xmlns:gedmo="http://gediminasm.org/schemas/orm/doctrine-extensions-mapping"
                  xsi:schemaLocation="http://doctrine-project.org/schemas/orm/doctrine-mapping https://www.doctrine-project.org/schemas/orm/doctrine-mapping.xsd">
    <entity repository-class="Core\Repository\Asset\AssetUsageReadingRepository"
            name="Core\Entity\Asset\AssetUsageReading"
            table="asset_usage_readings">
        <id name="takenAt" type="datetime_id"/>
        <id name="vehicle" association-key="true"/>

         <!-- other irrelevant fields -->

    </entity>
</doctrine-mapping>

datetime_id is a custom type extension of Doctrine\DBAL\Types\DateTimeTzType, as I needed to override the format and __toString methods of \DateTime

use DateTime;

class DateTimeIdentifier extends DateTime
{
    public const FORMAT = DATE_ISO8601;

    public function __toString()
    {
        return $this->format();
    }

    public function format($format = null)
    {
        return parent::format($format ?? self::FORMAT);
    }
}

@greg0ire
Copy link
Member

greg0ire commented Jan 16, 2022

Might be fixed by #1229 . Please try out 3.4.1 and doctrine/dbal 3.3.1

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

4 participants