[5.6] Temporal columns improvements (broken by GitHub)

[paulofreitas]

I was unsure if I have to push this to 5.5 or 5.6, but since it affects only new created columns and fixes/improves a lot of expected behavior I guess it would be wanted for 5.5.

If intended I can rebase this to 5.6 or to revert some unwanted change. 👍

Once again my contribution ended up big and I had to split it with 4 commits to ease the review - it's a lot easier to see the changes separately. I also prepared an extensive summary of changes to help reviewing. 😃

Part 1: Ensure unsupported temporal columns reuse supported ones

Basic code cleanup and consistency improvement. No tests changes yet.

Changes:

• Illuminate\Database\Schema\Grammars\MySqlGrammar
  • Alias typeTimeTz() to typeTime()
• Illuminate\Database\Schema\Grammars\PostgresGrammar
  • Alias typeDateTime() to typeTimestamp()
  • Alias typeDateTimeTz() to typeTimestampTz()
• Illuminate\Database\Schema\Grammars\SQLiteGrammar
  • Moves typeTimestamp() implementation to typeDateTime() and alias typeTimestamp() to typeDateTime()
  • Alias typeDateTimeTz() to typeDateTime()
  • Alias typeTimeTz() to typeTime()
  • Alias typeTimestampTz() to typeDateTimeTz()
• Illuminate\Database\Schema\Grammars\SqlServerGrammar
  • Moves typeTimestamp() implementation to typeDateTime() and alias typeTimestamp() to typeDateTime()
  • Moves typeTimestampTz() implementation to typeDateTimeTz() and alias typeTimestampTz() to typeDateTimeTz()
  • Alias typeTimeTz() to typeTime()

Benefits:

• Reduces code duplication and future mistakes
  • By aliasing unsupported columns to supported ones, a change to a supported column would also change the unsupported one, reducing mistakes that have been introduced in the past
• PostgreSQL/SQLite/SQL Server: allows handling useCurrent() on dateTime() and dateTimeTz() columns (this is improved later)

Part 2: Ensure temporal columns handle precision correctly

Improves temporal columns precision handling with full SQL standard compliance. Adds 8 new tests and 76 assertions.

Changes to code:

• Illuminate\Database\Schema\Blueprint
  • Continues the work from PR [5.5] Add precision to time #21936 adding $precision argument to the missing timeTz() method
• Illuminate\Database\Schema\Grammars\MySqlGrammar
• Illuminate\Database\Schema\Grammars\PostgresGrammar
• Illuminate\Database\Schema\Grammars\SqlServerGrammar
  • Fixes typeTime() and typeTimeTz() to correctly handle the given column precision
  • Simplifies some temporal columns implementation where possible

Changes to tests:

• Illuminate\Tests\Database\DatabaseMySqlSchemaGrammarTest
• Illuminate\Tests\Database\DatabasePostgresSchemaGrammarTest
• Illuminate\Tests\Database\DatabaseSQLiteSchemaGrammarTest
• Illuminate\Tests\Database\DatabaseSqlServerSchemaGrammarTest
  • Minor improvements to method names, column names and tests ordering
  • Ensured all existing tests also tests passing a custom precision to temporal columns where it's expected to handle
  • Added missing testSoftDeletes() and testSoftDeletesTz() tests

Benefits:

• SQL standard compliance with full support for passing a floating-point precision to TIME, DATETIME and TIMESTAMP columns
• Correctly use 0 precision in all drivers when no precision is used
• More code cleanup
• Improved test suite

Part 3: Ensure all temporal columns handle useCurrent() as supported

Improves all temporal columns to handle the useCurrent() column modifier as supported by the database driver in use. Adds 28 new tests and 56 assertions, drops 3 existing tests and 12 assertions.

Changes to code:

• Illuminate\Database\Schema\Grammars\Grammar
  • Added a new setTemporalDefaut() protected method
• Illuminate\Database\Schema\Grammars\MySqlGrammar
• Illuminate\Database\Schema\Grammars\PostgresGrammar
• Illuminate\Database\Schema\Grammars\SQLiteGrammar
• Illuminate\Database\Schema\Grammars\SqlServerGrammar
  • Changed temporal columns to call the new setTemporalDefault() method
  • Simplified temporal columns code removing the inline useCurrent handling*

Why I have changed this to use a new method? Because it simplifies things and also ensures the SQL grammars always generate DEFAULT expressions after NULL/NOT NULL expressions. All current drivers support DEFAULT expressions in either position but their CREATE TABLE documentation syntax always place DEFAULT expressions after NOT/NOT NULL expressions - thus we can say it's less risky to break if for any reason they change this behavior.

Changes to tests:

• Illuminate\Tests\Database\DatabaseSchemaBlueprintTest
  • Drops existing testDefaultCurrentTimestamp() method in favor of new grammar-specific tests
• Illuminate\Tests\Database\DatabaseMySqlSchemaGrammarTest
  • Drops existing testAddingTimestampWithDefault() andtestAddingTimestampTzWithDefault() methods in favor of new grammar-specific tests
• Illuminate\Tests\Database\DatabaseMySqlSchemaGrammarTest
• Illuminate\Tests\Database\DatabasePostgresSchemaGrammarTest
• Illuminate\Tests\Database\DatabaseSQLiteSchemaGrammarTest
• Illuminate\Tests\Database\DatabaseSqlServerSchemaGrammarTest
  • Adds new tests for useCurrent() to all temporal columns

Benefits:

• Improved support for using the useCurrent() column modifier on supported temporal columns
• More code cleanup
• Improved test suite

Part 4: Improve temporal columns docblocks

Misc docblocks improvements to temporal columns. We're done. 😅

Benefits:

• Improved code documentation

If I didn't miss anything, it's all good. Tests are passing. 👍

[sisve]

I'll be the annoying guy that talks about breaking changes. I've got years worth of migrations, and if any of these migrations are generating another database structure than at the time they were initially run, then it's a breaking change. I have much higher requirements on the stability of migrations than I have on other parts of the code; the migrations works with the production database that lives for years even when the code base can be rewritten at will.

A quick check for breaking changes; reset your database (rollback+migrate) and see if your local development environment exactly matches the production database that has executed the migrations one-at-a-time over a long period of time. Think years or several major Laravel releases when thinking "long period of time".

Your changes to SqlServerGrammar::typeDateTime and typeTimestamp will drop our SQL Server 2005 support since it does not support datetime2.

How about introducing a $table->datetime2(...) instead?

[paulofreitas]

@sisve If I'm not wrong, as I commented on laravel/ideas#870, researching MSDN, Wikipedia and this top Google ranked source I've found that all these column types come from SQL Server 10.0 (2008): DATE, DATETIME2, DATETIMEOFFSET, GEOGRAPHY and TIME

If intended I can rebase this to 5.6 or to revert some unwanted change. It should be noted on the second commit that some implementations was using column precision even when not being used (I've not tested but I guess this would break code), some was respecting column precision when using useCurrent() and not otherwise, some was truncating TIME and not DATETIME, etc. All these inconsistencies got fixed together.

[paulofreitas]

Closing here temporarily to rebase the branch to revert the DATETIME2 change. 👍

[sisve]

We're now trapped in two parallel discussions, this PR and the linked internals issue.

Anyhow, I would help determining the scope of this PR if we could list all resulting database changes. I'm more worried about changes into the generated database than what method is now aliasing something else.

We've already found one, $table->datetime('test') is changed from datetime to datetime2(0) for SQL Server. That's alone, ignoring what version of SQL Server we're using, is a huge breaking change. We're no longer able to reproduce an existing production database locally by running the identical migrations, the same migrations that production has executed.

I would prefer if this list of changes was empty; that would cause the least amount of issues when upgrading. It's my personal view that the list must be empty if you're targetting 5.5, we can not change how people's migrations work in the middle of a release. Imagine the support nightmare; "Hey, my coworker got a datetime2 column but I only got a datetime column. We're both on Laravel 5.5"...

It's my personal view that the list must be empty if you're targetting 5.6 too. This is where I believe I am a bit controversial; but I would not like to see any breaking change to any migration code. Migrations live a long time, and needs to be stable for an equally long time.

No matter what; the list of changes needs to be documented so they can go into changelogs/upgrade documentation if someone less strict that me merges it.

[paulofreitas]

Ouch, I've rebased a lot of times when the PR was opened, once I closed and rebased I cannot reopen again - what a shame, GitHub. 😒

I'll open a new PR now targeting 5.6 and with the DATETIME2 change reverted. 👍