[5.5] Better precision support on datetime columns

[paulofreitas]

TL;DR

• Currently we can't give a custom precision to time() and timeTz() columns as allowed by supported databases

• This PR adds this functionality to time() and timeTz() columns when explicitly giving a column precision for MySQL, PostgreSQL and SQL Server (SQLite does not support this)

• Due to some existing inconsistencies related to databases default precisions and Laravel's default behavior this will not be fully functional for SQL Server yet, but this is an already existing issue for SQL Server's datetime columns precision handling - a fix for this will be proposed later

• This PR also slightly improves the datetime columns code with no behavior changes and further improves the test suite on this subject to ensure we're testing those methods accordingly

Proposal

This was split from #22004.

Currently we have precision support on dateTime(), dateTimeTz(), timestamp() and timestampTz() columns. However, the SQL-92 standard (ANSI X3.135-1992 and ISO/IEC 9075:1992) allows us to also specify precisions on time columns and except for SQLite (which does not support columns precisions at all) all other database drivers does support them.^*

This was recently requested in #22048 where OP was thinking it was already there. I'm proposing that we start honoring the given precision on both time() and timeTz() columns so the framework follow the usage expectation.

^* MySQL added this to TIME, DATETIME and TIMESTAMP in 5.6.4

Implementation

This time I'm just changing the time() and timeTz() methods behavior when a precision is explicitly given, so it should match the developer expectation. Previously I was doing more, but I left the inconsistencies changes to a future work. This was made this way to make it easier to get it accepted and even be pushed to 5.5 if desired.

Some related code simplifications were done with no behavior changes - this was done to make things easier to understand. I've also rewritten related tests and I increased the test suite's testability on this subject. To ease reviewing code changes I've made a detailed summary below.

Changes

I'm targeting this for 5.5 first just because I'm not sure where it would be desirable since it's a feature that would be applied only to new installations and just adds a non-existing behaviour that matches the usage expectation. If this would be acceptable just for 5.6, let me know and I'll rebase it accordingly ASAP. 😉

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
  • Changed typeTime() to honor the given precision.
  • Aliased typeTimeTz() to typeTime().
  • Simplified typeTimestamp() implementation. (no behavior change)
  • Minor docblocks improvements.
• Illuminate\Database\Schema\Grammars\PostgresGrammar
  • Changed typeTime() and typeTimeTz() methods to honor the given precision.
  • Simplified typeTimestamp() and typeTimestampTz() implementations. (no behavior change)
  • Minor docblocks improvements.
• Illuminate\Database\Schema\Grammars\SQLiteGrammar
  • Aliased typeDateTimeTz() to typeDateTime().
  • Aliased typeTimeTz() to typeTime().
  • Simplified typeTimestamp() implementation. (no behavior change)
  • Aliased typeTimestampTz() to typeTimestamp().
  • Minor docblocks improvements.
• Illuminate\Database\Schema\Grammars\SqlServerGrammar
  • Changed typeTime() to honor the given precision.
  • Aliased typeTimeTz() to typeTime().
  • Simplified typeTimestamp() and typeTimestampTz() implementation. (no behavior change)
  • Minor docblocks improvements.

Tests

22 new tests, 34 new assertions, 10 unneeded assertions removed

• Illuminate\Tests\Database\DatabaseMySqlSchemaGrammarTest
• Illuminate\Tests\Database\DatabasePostgresSchemaGrammarTest
• Illuminate\Tests\Database\DatabaseSQLiteSchemaGrammarTest
• Illuminate\Tests\Database\DatabaseSqlServerSchemaGrammarTest
  • Added new testAddingTimeWithPrecision(), testAddingTimeTzWithPrecision(), testAddingDateTimeWithPrecision(), testAddingDateTimeTzWithPrecision(), testAddingTimestampWithPrecision() and testAddingTimestampTzWithPrecision() methods. Existing MySQL and SQL Server tests got rearranged.
  • Removed unneeded precision tests from testAddingTimestampWithDefault(), testAddingTimestampTzWithDefault(), testAddingTimestamps() and testAddingTimestampsTz() methods - there's no need to test this twice.
• Illuminate\Tests\Database\DatabaseSchemaBlueprintTest
  • Minor change to testDefaultCurrentTimestamp() to fix the PostgreSQL test. It was using CURRENT_TIMESTAMP(0) in typeTimestamp() but this was never needed - if you specify a precision on the column you aren't required to also declare it on the datetime function.

[Dylan-DPC-zz]

Actually, mssql doesn't take 0 as default precision. @sisve knows about it.

[paulofreitas]

@Dylan-DPC Yup, SQL Server does use a default of 7 fractional seconds on TIME, DATETIME2 and DATETIMEOFFSET columns and 3 fractional seconds on DATETIME columns: http://sqlfiddle.com/#!6/d27f4/1

Note however I'm not changing anything related to that - no existing behavior is being changed, both typeTime() and typeTimeTz() would still be using time as before if no precision is given to the column. I left everything related to these inconsistencies to a future proposal that would relate to SQL Server only. 👍

It indeed won't allow you to use an explicit precision of 0 for SQL Server, but this leads us to the inconsistencies previously discussed - in fact, dateTime(), dateTimeTz(), timestamp() and timestampTz() currently does not allow you to do that, so it will not be different. 😉

[sisve]

I imagine that the migration system would have a much easier time figuring out what the developer expects if the parameters had a default value of null, instead of 0. Null would be the "meh, don't care, surprise me" setting where 0 is the "I want precision of 0". I see no problems with changing the default value for precisions from 0 to null for all datetime types, but that is out-of-scope for this PR.

What I really want is the keep the backward compatibility for things like $table->time('col') to generate a time column, and $table->time('col', 0) if you really wanted a time(0). This is relevant to keep backward compatibility with old code that is not specifying permission, while still allowing people to specify a precision of zero if they really wanted.

Regarding the secret ritualistic knowledge I possess about SQL Server; the default precision for a time(x) is 7. The current implementation will block users from creating a time(0) column since the 0 is interpreted as a "no precision specified" at the moment. This has already been mentioned, so I'm not sure if this should be reclassified as a public ritualistic knowledge...

So, the only thing I want to change about this PR is the default value for $precision to null.

You linked the SQL 92 grammar, but it's not containing the Syntax Rules. These are important because it shows that the default precision of time and datetime differs. That's totally not relevant to this PR, but interesting none the less.

25) If <time precision> is not specified, then 0 is implicit. If
    <timestamp precision> is not specified, then 6 is implicit.

26) The maximum value of <time precision> and the maximum value of
    <timestamp precision> shall be the same implementation-defined
    value that is not less than 6. The values of <time precision>
    and <timestamp precision> shall not be greater than that maximum
    value.

Source: SQL92, section 6.1 <data type>, Syntax rules

[sisve]

Hey, I just realized, this PR is named to include all datetime fields so it's not at all out-of-scope to change the default value for them all to null. I was reading the diff and thinking it was only focusing on the time/timeTz methods. This means we can solve most things. Hah!

[taylorotwell]

These huge PRs with tiresomely long explanations are very hard to find the time to manage. We need good TLDR.

[paulofreitas]

@sisve What about moving that later requirement into a separated PR? Just to make this one "reviewable" - it has already a reasonable amount of lines changed to review. 😅

With this PR we will still be lacking support for specifying 0 as a datetime precision, but I guess it's fine for the scope of this PR since this functionality does not work already (at least for SQL Server). I'll tweak the PR title accordingly. 👍

I think it's still worth opening a new internals topic on this subject to discuss and summarize all the existing inconsistencies, their possible solutions, impacts, etc. What do you think? 😃

@taylorotwell I'll be providing a TLDR section to the PR description ASAP. 😉

[deleugpn]

TL;DR: Add support for custom precision when using time and timeTz columns for MySQL, Postgres and SQL Server. SQLite does not support it.