propelorm / Propel2

Propel2 is an open-source high-performance Object-Relational Mapping (ORM) for modern PHP
http://propelorm.org/
MIT License
1.26k stars 398 forks source link

allow datetime type columns #1890

Closed mringler closed 2 years ago

mringler commented 2 years ago

This allows to use column type DATETIME.

At the moment, while Propel handles DATETIME correctly in schema.xml and generates the expected migrations, building the model will lead to an error:

Error setting up column receipt_date: Cannot map unknown Propel type 'DATETIME' to native database type.

Instead, the type "TIMESTAMP" has to be used, which is fine for almost all DBMS, since usually only one datetime/timestamp type is available anyway.

However, in MariaDB, both types are available and they are notably different in that they support different date ranges, and that MariaDB sets default values for the first timestamp in a table (see here), making TIMESTAMP the type to use if you want to timestamp the row, while DATETIME is DATE + TIME.

So I think it make sense that the generator can handle both DATETIME and TIMESTAMP. Because while they are synonymous inside Propel, they might mean different things in schema.xml.

dereuromark commented 2 years ago

Only postgres doesnt seem to like it.

mringler commented 2 years ago

For some reason, Propel did not translate column type DATETIME to TIMESTAMP for Postgres, leading to invalid migrations:

There was 1 failure:
1) Propel\Tests\Generator\Migration\BaseTest::testColumnTypeChangeMoreComplex
Failed to apply the first/original schema:
Can not execute SQL: 
CREATE TABLE "migration"."migration_test_3"
(
    ...
    "field15" TIMESTAMP,
    "field16" DATETIME --  <------ This fails since DATETIME is not a valid column type in Postgres
)

This had to be a problem in PgsqlPlatform::getColumnDDL(), and I have no idea why it works now, but it does. Yay, I guess?

A bit last minute, since we wanted to tag the beta yesterday/today^^

I know, right? Ran into the problem yesterday evening shortly after having read your comment and thought the same thing.

mringler commented 2 years ago

Oh, just realized that people might get unexpected migrations if they use MySQL or MariaDB with TIMESTAMP/DATETIME columns. Because before, they had to use TIMESTAMP in schema.xml, which internally was converted to DATETIME. With these changes, DATETIME is DATETIME and TIMESTAMP is TIMESTAMP (which is not the same as DATETIME in MySql as well), so running propel diff will now see that columns declared as TIMESTAMP are currently DATETIME columns and create according migrations.

I don't think this qualifies as BC-break because functionality is not affected, and running the migrations should not cause any issues (even though adjusting the type in schema.xml might be preferred). This is a consequence from the current state, where a valid datatype cannot be used (TIMESTAMP) but its name refers to another type, which name (DATETIME) is not used at all. I don't see another way of fixing this, and I think overall it is worth it. But it might be good to warn about the behavior in the release notes.