[WIP] [5.6] Temporal columns improvements

[paulofreitas]

From PR #22000 (broken after rebasing with PR closed)

This was originally targeting 5.5, then I've changed to 5.6. If intended I can rebase this to revert some unwanted change. There was some ongoing discussion on PR #22000 and laravel/ideas#870. 👍

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. 😃

This PR description was a lot simplified after created to ease tracking its changes. Here's a summary of what it changes.

Part 1: Ensure unsupported temporal columns reuse supported ones

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

Changes to code - items marked with * changes the existing behavior:

• 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 - items marked with * changes the existing behavior:

• Illuminate\DatabaseQuery\Grammars\SqlServerGrammar
  • Fixes getDateFormat() to use Y-m-d H:i:s.u format instead (if may break the application if you use the useCurrent() modifier in a columns using the DATETIME column type)
• 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 (except in SQL Server DATETIME)
• 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 setDefaultTimeFunction() 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. 👍

[deleugpn]

Would be nice to have MySQL 5.5 support as a separate driver.

[paulofreitas]

@deleugpn Yeah, we're discussing this sort of things on laravel/ideas#870 - there's some good ideas there. 👍

[taylorotwell]

Can you explain the benefits provided by this PR?

[themsaid]

@paulofreitas can you please try to make your PRs as small as possible, it's extremely hard to understand the PR if it has this amount of line changes.

For example here you're changing lots of docblocks, you can do that in a separate PR that has the purpose of "Enhancing docblocks", that way we see only logic changes in the PR, and docblock changes in another.

[paulofreitas]

@taylorotwell Summarizing:

All drivers

• It fixes handling precision on time() and timeTz()
  • While being defined in the SQL standard and implemented by supported drivers (except SQLite), currently it's not possible to pass a precision to a time column - it's silently ignored (this was recently reported in Blueprint Builder doesnt Support TIME(3) // Milliseconds #21935)
• It adds handling of useCurrent() to date(), dateTime(), dateTimeTz(), time() and timeTz()
  • It's allowed by the SQL standard and, though supported drivers differs in the implementation, all them have support for implementing this
• It delegates CURRENT_* functions compilation to modifyDefault() method
  • While this does not matter to the supported database engines, this would at least improve consistency - with this change NULL/NOT NULL expressions always come before default values (currently testing useCurrent() and default() modifiers generates inconsistent output)
  • This also avoid mistakes of using both useCurrent() and default() column modifiers together (one will overwrite the other)
• It improves the test suite a lot (36 new tests and 132 assertions) ensuring future changes to temporal columns wouldn't break things
  • Ensures precisions and the use of useCurrent() modifier are handled correctly in all temporal columns where it's supported

SQL Server

• It fixes time(), timeTz(), dateTimeTz() and timestampTz() output to always use a default precision of zero where possible (see this SQL fiddle)
  • SQL Server defaults temporal columns precision to 7 decimal places (3 for DATETIME column type) and some temporal columns already uses zero under certain circumstances - this change aims to standardize the behavior of all supported drivers by using zero as temporal columns precision when no precision is used
  • I was about to change the dateTime() and timestamp() to use DATETIME2(0) when not declaring precision but @sisve reminded me that this was a intended behavior from [5.5] Bring back support for MySQL 5.5 and SQL Server 2005 #20465 to retain SQL Server 2005 compatibility - this raised some discussions on [Proposal] Decouple MariaDB support from MySQL ideas#870 about legacy drivers versions
  • I haven't changed dateTime() and timestamp() yet but I still believe they should also be changed for all the reasons explained in this comment. Using DATETIME2(0) would correctly fix all columns precisions: updated SQL fiddle

@themsaid I pushed the commits without squashing to ease reviewing, it's a lot easier to see the commits changes separately. I've summed up the changes above. But I'll do my best to make future PRs much smaller. 👍

Edit note: I've made some simplifications to this PR after your comments. Both PR code and description got reduced/improved. :)

[sisve]

To both retain SQL Server 2005 compatibility and use a default precision of zero when not declaring precision I've changed dateTime() and timestamp() to use SMALLDATETIME instead

These are the things that are huge breaking changes. Every soft-deletable model and datetime column gets a new data type.

Do we agree that our dev databases should match the production database? If so, how should the dev database be migrated if running the migrations in 5.6 doesn't create a database like they did when they were executed in production (which was running 5.5 at the time)?

[paulofreitas]

Sorry for making a broad answer here, but I think I should explain the issue in detail.

These are the things that are huge breaking changes. Every soft-deletable model and datetime column gets a new data type.

Actually I was fooled by my own tests: SMALLDATETIME should not be used, it resets seconds to zero. And sticking to DATETIME is not a good choice either. Before going deeper into the reasons, let me clarify some things.

There are more things changing here and they're both breaking changes and bug fixes - that's why I changed my mind and retargeted the PR base branch to 5.6.

We should promise that 5.5 won't break in a point release, but we shouldn't promise that a new major version won't affect existing code - this way we'll never be fixing broken things and/or improving existing behavior. It should be noted that we're not following SemVer and this is allowed by the current versioning policy: https://laravel.com/docs/5.5/releases#versioning-scheme

Okay, come on. I've reverted SMALLDATETIME back to DATETIME just to avoid further confusion - but it's not good yet. Let me show what's changing for SQL Server at this moment: http://sqlfiddle.com/#!6/11091/1

Did you see datetime_noprecision and timestamp_noprecision? I think we still need to change that. Why?

Because they aren't matching expectations. You create a temporal column without declaring any precision and they get stored with a default database precision for the column (7 fractional seconds for TIME and DATETIMEOFFSET and 3 for DATETIME). Don't you agree with me that this is broken?

Now look the issue #21870 reported recently. It shows us that the timestamp() column is currently broken on Laravel 5.5 if you use the useCurrent() column modifier. Either Illuminate\Database\Schema\Grammars\SqlServerGrammar->compileTimestamp() should be using CURRENT_TIMESTAMP(0) or Illuminate\Database\Query\Grammars\SqlServerGrammar->getDateFormat() should be using Y-m-d H:i:s.u instead. I'll patch this as soon as I publish my comment.

The point here is that people shouldn't be expecting any fractional seconds in those columns once all other schema grammars behaves differently and uniformly. We store time/date-time/timestamps columns without declaring any precision and we get timestamps with fractional seconds. Let me show you an experiment I did with SQL Server 2016 using Amazon RDS:

Migration

/* ... */

Schema::create('schedules', function (Blueprint $table) {
    $table->increments('id');
    $table->date('schedule_date');
    $table->time('time_start');
    $table->time('time_end');
    $table->timestamps();
    $table->timestamp('time_created')->nullable()->useCurrent(); // this is just an POC example
});

/* ... */

Model

/* ... */

class Schedule extends Model
{
    protected $fillable = ['schedule_date', 'time_start', 'time_end'];

    // protected $dates = ['time_created'];
    // Purposely not using Carbon for time_created
}

/* ... */

Seeder

/* ... */

class SchedulesTableSeeder extends Seeder
{
    public function run()
    {
        Schedule::create(['schedule_date' => '2017-11-13', 'time_start' => '04:00:00', 'time_end' => '09:00:00']);
    }
}

/* ... */

Route

/* ... */

Route::get('/', function () {
    $schedule = Schedule::find(1);
    var_dump($schedule->time_start); // string(16) "04:00:00.0000000"
    var_dump($schedule->time_end); // string(16) "09:00:00.0000000"
    var_dump($schedule->time_created); // string(23) "2017-11-13 06:25:52.150"
    var_dump((string) $schedule->created_at); // string(19) "2017-11-13 06:25:52"
    var_dump((string) $schedule->updated_at); // string(19) "2017-11-13 06:25:52"
});

/* ... */

Currently if I change Schedule::$dates to include time_created I simply break the application: InvalidArgumentException: The format separator does not match. This is fixable as mentioned earlier, but look that if you don't use Carbon, your timestamps don't look nicely. And what about your time columns? They don't use Carbon. I think it's time to make this behavior be consistently across the database drivers.

There's a conflict of interest here: either you provide support for SQL Server 2005 or you fix all the inconsistencies uniformly. This raises the discussion of providing alternate drivers for legacy database engines we started at laravel/ideas#870. People using SQL Server 2008+ would expect Laravel to behave exactly as shown here: http://sqlfiddle.com/#!6/a22af/1

If the idea of providing separated legacy drivers aren't desirable, maybe we could at least provide a Blueprint option (like $table->enableLegacyColumns or another name) so we can change Blueprint->dateTime()and Blueprint->timestamp() to pass this option to the grammars and thereby the SqlServerGrammar could be using DATETIME2(0) by default and DATETIME if the option is enabled. 😃

I believe that to this PR be complete we should be using DATETIME(0) in SQL Server dateTime() and timestamp() columns as it was earlier proposed months ago. Being SQL Server 2005 a product not supported by Microsoft anymore I believe it's more reasonable to provide some limited support through Blueprint's option than being behaviorally inconsistently just to provide this legacy support out-of-the-box. 👍

[sisve]

We should promise that 5.5 won't break in a point release, but we shouldn't promise that a new major version won't affect existing code - this way we'll never be fixing broken things and/or improving existing behavior.

In the context of migrations; why cannot we make such a promise?

I accept that there are code changes in other parts of the framework, and there are usually documented with "if you did this before, do this now" to get the same behavior. Why cannot we do the same here? And if we still can keep the old behavior, why do we need to change existing methods when we can just introduce new methods for the new behavior?

Because they aren't matching expectations. You create a temporal column without declaring any precision and they get stored with a default database precision for the column (7 fractional seconds for TIME and DATETIMEOFFSET and 3 for DATETIME). Don't you agree with me that this is broken?

I do not think that this behavior is broken. You did not specify a permission, but let the default decide. So you get the default. The default varies on different platforms, but that is now your problem since you accepted the default.

The linked issue (#21870) does mention the actual cause, that this is some shenanigans that is caused by the sqlsrv pdo driver. It's not totally unheard of. microsoft/msphpsql#284

I accept that $table->time('zomg', 0) would create a TIME(0), but do we really have to break old invocation of $table->time('zomg') creating a TIME, like #21936 did?

[sisve]

Okay, I'll admit defeat and give up this discussion. I just found out that 5.5 basically broke every temporal column already with their default of $precision = 0 instead of $precision = null. I'll instead move away to a non-Laravel based migration system, I feel like it is a full time job to keep track of ways that Laravel may mess up my database structure.

[paulofreitas]

In the context of migrations; why cannot we make such a promise? [...] Why cannot we do the same here? And if we still can keep the old behavior, why do we need to change existing methods when we can just introduce new methods for the new behavior?

If you are fixing an unexpected behavior, the promise could be broken when you bump the major version.

Actually #21936 didn't changed anything - currently the schema grammars do not honor any given precision. This PR, on other hand, changes that in the way it was intended in #21936.

Look for example that #18847 was added to Laravel 5.5.0 (it got partially reverted/fixed) and changed the existing behavior - it was an improvement, so it's desirable. Earlier Laravel 5.0.0 also changed a lot in 0c0a717 to improve the framework. We can't restrict people from using new features in newer major versions because someone coming from a previous major version does not want to have any changes when upgrading - the only difference you have by following SemVer is that a change like that would require Laravel 5.6 to be versioned Laravel 6. Even following SemVer you're allowed to break things like that if you're improving the software behavior. 👍

Let me give you another example: currently we could've full support for ENUM columns in all database drivers but it's fully implemented for MySQL and PostgreSQL only. If I'd propose this to also being added to Laravel 5.6 to improve SQLite/SQL Server functionality by adding aCHECK constraint to them (they both support this as PostgreSQL does), I shouldn't? This is quite wrong. We should promise things won't break only and just only the version you've coded your application - if you won't be upgrading because something changed and require maintenance changes, well, this is up to you. You could simply don't upgrade instead. This is what happens for almost all software we use, don't?

And if we still can keep the old behavior, why do we need to change existing methods when we can just introduce new methods for the new behavior?

This is kinda counterintuitive IMO. This way you're simply bloating your API. 😐

I do not think that this behavior is broken. You did not specify a permission, but let the default decide. So you get the default. The default varies on different platforms, but that is now your problem since you accepted the default.

That's a good reason to have a decent default that don't confuse people IMO. After all you won't be documenting every single difference a database have to another in a framework pretending to simplify your daily coding. 😄

Please do note that I'm just proposing a complete behavior fix to actually don't have to change this anymore in the future. 👍

[taylorotwell]

I literally have no idea what is even happening in this PR.

[paulofreitas]

@taylorotwell I'll make it easy to review by splitting each feature addition/change being proposed into separate PRs. 👍