Add support for variable precision TIMEs and DATETIMEs #5961

cgknx · 2023-03-09T11:02:03Z

Q	A
Type	feature
Fixed issues	#2873

Summary

Adds support for high time precision times and datetimes where supported by the platform.

The feature is enabled by default for all platforms except SQLite and Oracle where the feature is opt-in (see below). Neither Oracle nor db2 can support high precision time columns without changing the column type used and so only high precision datetimes are supported on these platforms.

Use

To add high precision time to a column, add column['scale'] to the column specification to specify the number of decimal digits to include in the time. The maximum is platform specific. Since php's datetime objects hold a maximum of six decimal places, this is the effective practical maximum scale. Where fewer than six decimal digits are specified for a column, the time will be truncated or rounded depending on the platform.

Notes

PHP returns an error attempting to convert, for example, a time such as '10:10:10' (without decimal seconds) to a datetime object with a format of 'H:i:s.u' (with decimal seconds). To deal with columns specified as just DATETIME or DATETIME(0), there is a fallback attempt to convert using 'H:i:s' should 'H:i:s.u' fail.

SQLite

SQLite stores DATETIMEs as strings which can have arbitrary precision irrespective of column definition. This means a table could return any number of decimal places of precision for rows of one particular DATETIME column. This may be an issue because 10:10:10 != 10:10:10.0 as far as SQLite is concerned. Use of high precision times is therefore opt-in for SQLite.

To enable high precision datetimes and times in SQLite, add driverOption['high_precision_timestamps'] = true.

This driver option selects between the existing SQLitePlatform and a new SqliteHptPlatform which uses a high precision time format string.

Oracle

The format of datetimes returned by the database to DBAL is configured in driver middleware. The format does not include decimal digits. A driver configuration parameter has been added to change the underlying datetime format and enable high precision times.

To enable high precision datetimes and times in Oracle, add driverOption['high_precision_timestamps'] = true.

This driver option configures the middleware to add six decimal digits to NLS_TIMESTAMP_FORMAT and NLS_TIMESTAMP_TZ_FORMAT (i.e. .FF6) and selects a new OracleHptPlatform which uses high precision time format strings.

derrabus · 2023-03-13T13:40:48Z

Thank you for this PR. We might have handled precision on date fields a little inconsistently in the past. If we merge your PR, we should make sure we've taken all drivers into account and not just MySQL and Postgres.

SQL Server for instance also supports setting the precision on datetime2 fields. As far as I understand, we already set the precision to a value of 6 (although the default and maximum would be 7):

dbal/src/Platforms/SQLServerPlatform.php

Line 1375 in c982b9e

return 'DATETIME2(6)';

Oracle also supports a precision ranged from 0 to 9, but we hardcoded it to 0:

dbal/src/Platforms/OraclePlatform.php

Line 322 in c982b9e

return 'TIMESTAMP(0)';

DB2 supports a precision between 0 and 12 which we also hardcode to 0:

dbal/src/Platforms/DB2Platform.php

Line 299 in c982b9e

return 'TIMESTAMP(0)';

Can you look into how your changes would play with how we treat datetime columns on SQL Server and Oracle and DB2 currently? Unless I've missed something, this basically means that we don't need the supportsVariablePrecisionTime() override because all of the platforms are.

Another question: What would happen if we set the precision to a value greater than what the current platform supports, e.g. I set it to 9 and try to deploy that schema on a Postgres server?

src/Platforms/AbstractPlatform.php

tests/Functional/Types/DateTimeTest.php

cgknx · 2023-03-13T20:06:51Z

I'd seen the inconsistencies in terms of sometimes setting 0, 6 or no precision and took that into account with the patch set. I believe it deals with things like DATETIME2(6) and TIMESTAMP(0) correctly and sets the precision based on the length field in all cases although I don't have test systems to use. I can look at extending support to other databases relying on the CI checks to identify issues. I may need help debugging them though!

I used a supports flag so platforms can be added incrementally. If all have support then we can remove the supports flag and simplify the methods that depend on it as a final patch in the series (or a later patch).

In particular and as I noted in the PR, I'm not convinced SQLite's datetime precision support is robust and worry it will lead to issues. My understanding is that a particular column would store (string) datetimes from before the upgrade with no precision and those after the upgrade with microsecond precision. The inconsistency bothers me.

Finally, if we set datetime precision to 9 on a platform that doesn't support it, the maximum valid precision for the relevant platform would be used. No error would be generated so the schema you described would deploy albeit with a lower precision.

cgknx · 2023-03-13T20:08:01Z

I'll look at turning on support in other platforms later this week but hope I've addressed your first set of comments.

derrabus · 2023-03-13T22:07:15Z

I don't have test systems to use.

You can easily spin up each of the supported databases with docker. That's what our CI does as well.

tests/Functional/Types/DateTimeTest.php

cgknx · 2023-03-17T14:11:47Z

Support added for DB2, SQL Server and Oracle. Neither DB2 nor Oracle support high precision Time types because of the choice of column type and it is not obvious how to add such support in a BC manner (the commit message explains the detail).

Support not added for SQLite given potential BC issues already flagged and so supportsVariablePrecisionTime() is still used.

I'll squash the patchset down and rebase if necessary once you've had a chance to review.

src/Platforms/AbstractPlatform.php

morozov · 2023-03-24T20:47:05Z

src/Platforms/AbstractPlatform.php

+    protected function addVariablePrecisionTimeSQL(string $decl, array $column): string
+    {
+        if (
+            ! isset($column['length']) ||


Why is column precision defined by the "length" attribute?

The PR description explains it:

'Precision' or 'scale' could have been used but existing documentation says these are reserved for DECIMAL types so 'length' was chosen as it seems to be parsed for all column types.

But "precision" would indeed look like a better fit imho. The documentation can (and should) be changed.

Precision would be a better linguistic fit but scale would be correct, I think (precision being number of significant digits, and scale the number of decimal places).

Both precision and scale are defined in the Column class as ints with defaults set whereas length is defined as int|null with no default. Since this PR uses undefined length to mean the historical default (0, 6 or null depending on platform) using length was a pragmatic solution to avoid BC breakage. Also I recall when I tested this with Symfony, precision and scale defaulted to 10 and 2 which would also lead to a change to the schema and incorrect migration.

So I went back to length as the only viable option I could think of. Make sense?

Not really. Why do precision and scale default to anything for datetime fields at the moment? I think, we can change those defaults without a BC break as long as they remain the same for decimals.

I'll have another look as it's been a while.

src/Platforms/AbstractPlatform.php

morozov · 2023-03-24T20:48:28Z

tests/Types/VarDateTimeImmutableTypeTest.php

-            ->with('Y-m-d H:i:s')
-            ->willReturn('2016-01-01 15:58:59');
+            ->with('Y-m-d H:i:s.u')
+            ->willReturn('2016-01-01 15:58:59.000000');


Changes in existing tests often identify breaking changes.

Even in a mock configuration?

Side note: Do we actually need this mock? 🫣

In this case, do we need this change?

There is definitely a change to behaviour - the format string now has a microseconds specifier and the string presented to the database has six decimal digits. The functional tests test all precisions of datetime (including zero) against all column precisions to ensure the behaviour is as expected.

Whether or not we use the mock, there will need to be a change to the assertion in the test to reflect the additional decimal digits in the datetime string.

tests/Functional/Schema/VariableDateTimePrecisionIntrospectionTest.php

morozov · 2023-03-24T20:53:02Z

tests/Platforms/DB2PlatformTest.php

        self::assertEquals('TIME', $this->platform->getTimeTypeDeclarationSQL($fullColumnDef));
    }

+    public function testGeneratesTypeDeclarationForDatetimes(): void


I would deliberately not introduce unit tests for SQL. They produce code coverage but don't test interaction with the database. It's better to make sure that all the new behaviors are tested by integration tests.

I deliberately added them to ensure that where no length (or a zero length) is provided, the outcome is the SQL declaration that is currently generated. For instance the current defaults are DATETIME2(6) and TIME(0) on SQL server but DATETIME or TIMESTAMP and TIME on MySQL. The MySQL versions would be equally valid on SQL server but result in a migration if they were used. So I do think they add something (and they picked up an error when updating the patch set).

If you still think they are useless, I can delete them.

cgknx · 2023-03-28T18:46:06Z

Additional patches added to reflect comments above. @derrabus, @morozov, let me know if you'd like me to squash them into the original patches before you take a look.

morozov · 2023-03-28T18:31:08Z

src/Driver/OCI8/Middleware/InitializeSession.php

+                        . " NLS_TIMESTAMP_FORMAT = 'YYYY-MM-DD HH24:MI:SS.FF6'"
+                        . " NLS_TIMESTAMP_TZ_FORMAT = 'YYYY-MM-DD HH24:MI:SS.FF6 TZH:TZM'"


It looks like a BC break.

I'm not familiar enough with Oracle to comment with any authority. The DateTimeTest sends times to the database in formats including the former ''YYYY-MM-DD HH24:MI:SS' (i.e. without FF6) so it seems Oracle is sane enough to ignore the missing decimals. The db also always passes timestamps to the dbal with six decimal digits irrespective of column width. However, the conversion to the datetime object, php doesn't care how many digits actually exist so the extra zeroes should have no effect. Am I missing something in particular you think could be dealt with differently?

It is a BC break unfortunately. If the app uses the type system to parse a date coming from the server, we should be fine. But if the app operates on the query result directly, this change causes datetimes to be returned in a different format which the app would not expect. We need to make this change of behavior opt-in, I'm afraid.

What do you think about using YYYY-MM-DD HH24:MI:SSFF as the format string (i.e. no decimal point and no number of digits specifier). My understanding is that this will then behave just like 'YYYY-MM-DD HH24:MI:SS' for a TIMESTAMP column with no decimal digits (and no TIMESTAMP column can have decimal digits with the existing NLS_TIMESTAMP_FORMAT). We would then also need to drop the decimal point in the php format string but that's trivial. The test suite passes with this change after ensuring we compare php datetime objects rather than string representations in the new DateTime functional test.

That feels like a hack because it would tell Oracle to communicate timestamps in a rather unusual format. Since the middleware has to be wired explicitly by the app, I think opting into the more precise timestamp format would be easily doable. Make it a boolean constructor flag and trigger a deprecation when it's not enabled. In 4.0, we can remove the flag again.

Easily doable? Well, I've tried but maybe I've missed a shortcut and made it much too complicated. I believe the middleware or a deprecated event listener sets the session variables that define timestamp formats in Oracle. The platform has to know these session variables to ensure the php datetime format strings match exactly but as far as I can see, the platform knows nothing of the driver. So I've got the connection querying the db (if the driver is Oracle) to determine the value of the session variables (because if set by the listener, the driver won't know, will it?) and passing a flag to platform when it instantiates it. Since the middleware is wired by TestUtil, I'm not sure how to get the CI to test the high precision variant although it passes the testsuite locally. Let me know what you think (or whether we just say Oracle won't support high precision datetimes until 4.0?). I'm unlikely to be able to look at this again for a week or so.

morozov · 2023-03-28T18:40:29Z

src/Platforms/AbstractPlatform.php

+        string $lengthIfZero = '',
+        string $lengthIfNull = '',
+        string $suffix = '',
+        int $maxPrecision = 6


Why are these defaults needed and where does the 6 come from? None of the callers seem to pass any other value of $maxPrecision.

Obviously not necessary but intended to be indicative of sane defaults. Max precision of 6 reflects php's internal maximum precision which I've suggested is adopted for all platforms.

morozov · 2023-03-28T18:41:11Z

src/Platforms/AbstractPlatform.php

+     */
+    protected function buildDateTimeTypeDeclarationSQL(
+        array $column,
+        string $decl,


The base SQL for the column type e.g. TIMESTAMP or TIME

Should it be $typeDeclaration then?

Probably. I can change it.

morozov · 2023-03-28T18:41:55Z

src/Platforms/AbstractPlatform.php

+        string $suffix = '',
+        int $maxPrecision = 6
+    ): string {
+        if (! isset($column['length']) || ! is_numeric($column['length'])) {


Why do we expect a non-numeric length?

We shouldn't expect it but don't we want to sanitize input? I can delete the check.

morozov · 2023-03-28T18:42:08Z

src/Platforms/AbstractPlatform.php

+        int $maxPrecision = 6
+    ): string {
+        if (! isset($column['length']) || ! is_numeric($column['length'])) {
+            return trim($decl . $lengthIfNull . ' ' . $suffix);


Why does the result need to be trimmed?

To remove the trailing space when $suffix is null (to ensure absolutely consistent with what is currently generated).

morozov · 2023-03-28T18:43:18Z

src/Platforms/AbstractPlatform.php

+        string $lengthIfZero = '',
+        string $lengthIfNull = '',
+        string $suffix = '',


This API seems to be designed to optimize code reusability but it's quite hard to understand. I'd rather have one method per use case than one method with cryptic parameters.

I do take your point but so I'm absolutely clear, are you saying you'd prefer platform specific methods to generate SQL declarations for each of datetime, datetimetz and time (with not implemented versions in the AbstractPlatform)? If you confirm, I can do that.

morozov · 2023-03-28T18:46:59Z

tests/Platforms/AbstractPlatformTestCase.php

+    public function testGetDateTimeFormatString(): void
+    {
+        self::assertEquals('Y-m-d H:i:s.u', $this->platform->getDateTimeFormatString());
+    }
+
+    public function testGetFallbackDateTimeFormatString(): void
+    {
+        self::assertEquals('Y-m-d H:i:s', $this->platform->getFallbackDateTimeFormatString());
+    }
+
+    /**
+     * Test datetime format with a timezone.
+     *
+     * Override this method where the time format includes a timezone e.g. PostgreSQL, SQL Server and Oracle.
+     */
+    public function testGetDateTimeTzFormatString(): void
+    {
+        self::assertEquals('Y-m-d H:i:s.u', $this->platform->getDateTimeTzFormatString());
+    }
+
+    /**
+     * Test fallback datetime format with a timezone.
+     *
+     * Override this method where the time format includes a timezone e.g. PostgreSQL, SQL Server and Oracle.
+     */
+    public function testGetFallbackDateTimeTzFormatString(): void
+    {
+        self::assertEquals('Y-m-d H:i:s', $this->platform->getFallbackDateTimeTzFormatString());
+    }
+
+    public function testGetTimeFormatString(): void
+    {
+        self::assertEquals('H:i:s.u', $this->platform->getTimeFormatString());
+    }
+
+    public function testGetFallbackTimeFormatString(): void
+    {
+        self::assertEquals('H:i:s', $this->platform->getFallbackTimeFormatString());
+    }


What do these tests test? It's better to have this logic tested via integration tests.

I agree they don't do a great deal in most cases. I'll delete them. The (non-Timezone) methods are already covered by functional tests.

morozov · 2023-03-28T18:49:22Z

src/Platforms/AbstractPlatform.php

+     * Conversion will fail where the format string returned by getDateTimeFormatString() contains a
+     * microsecond specifier (.u) but the string to be converted has no decimal digits.


When is this supposed to happen?

It's not supposed to happen in the sense that it is by design. It is how php converts datetimes, i.e. it does happen and there's nothing that I can see we can do about it other than use a different format string. I obviously wasn't clear enough in the comment.

cgknx · 2023-03-30T20:28:49Z

Revised patchset with SQL declaration generation now reworked into lots of simpler methods. Additional functional tests added to test times and datetimes with timezone (these picked up an error in the existing Oracle platform which is fixed). Removed sanitization of $column['length'], instead leaving the db to generate the error (like excess precision with decimals).

derrabus · 2023-04-03T18:49:40Z

src/Connection.php

+        if ($this->oracleConnectionSupportsHighPrecisionTimestamps()) {
+            $platformConfig['oracle_connection_supports_high_precision_timestamps'] = true;
+        }


Platform-specific code inside the generic wrapper classes is a no-go, sorry.

Reworked using a driver option instead.

derrabus · 2023-04-03T18:59:05Z

src/Connection.php

+        return $this->_driver instanceof AbstractDriverMiddleware &&
+               $this->_driver->wrappedDriverInstanceOf(AbstractOracleDriver::class);


This won't work. The driver stack can be arbitrarily deep, but apparently you're assuming a depth of 0 or 1. In addition to this,AbstractDriverMiddleware is only a helper for middleware implementations. It's not mandatory that a middleware extends it. The only contract is the Driver interface.

There is no generic way to unwrap the driver stack and that's on purpose. Whatever you're trying to do here: unwrapping the driver stack is a dead end.

Reworked using a driver option instead.

derrabus · 2023-04-03T18:59:54Z

src/Driver.php

     * @return AbstractPlatform The database platform.
     */
-    public function getDatabasePlatform();
+    public function getDatabasePlatform(array $platformConfig = []);


This is a breaking change. Adding this parameter breaks every downstream driver implementation, just like it broke all bundled drivers (which you've fixed already).

Reworked using a driver option instead.

derrabus · 2023-04-03T19:04:56Z

src/Event/Listeners/OracleSessionInit.php

@@ -38,8 +38,13 @@ class OracleSessionInit implements EventSubscriber
    ];

    /** @param string[] $oracleSessionVars */
-    public function __construct(array $oracleSessionVars = [])
+    public function __construct(array $oracleSessionVars = [], bool $useHighPrecisionTimestampFormatString = false)


I don't think we need to patch this deprecated event listener. Application that want to opt into the more precise timestamp format should simply switch to the middleware.

Opt-in for oracle and db2 now refactored. I've kept the event listener patch as there's no need to touch the constructor now. Happy to revert this bit of the oracle patch if necessary.

cgknx · 2023-04-11T10:58:39Z

Oracle opt-in to high precision time now selected using a driver option to configure the middleware and platform selection by the driver (i.e. I'm proposing separate platforms for legacy and high precision times differing only in php datetime format strings). Same option used to add optional sqlite high precision time support so all platforms now supported.

simPod

Overall LGTM, nice work!

I compared it to what I've tried to achieve in #6007 and have nothing much to add.

Some nits:

simPod · 2023-04-14T07:43:43Z

src/Platforms/OracleHptPlatform.php

+    /**
+     * {@inheritDoc}
+     */


Suggested change

/**

* {@inheritDoc}

*/

I think there's no need to add this inherit phpdoc since the inheritance is already implicit

I added these useless comments to stop phpcs complaining "Method \Doctrine\DBAL\Platforms\OracleHptPlatform::getTimeFormatString() does not have return type hint nor @return annotation for its return value. (SlevomatCodingStandard.TypeHints.ReturnTypeHint.MissingAnyTypeHint)".

src/Platforms/AbstractPlatform.php

Test conversion of datetimes and times including and excluding microseconds in the datetime string to php Datetime objects.

Types/DateTimeTest: Types/DateTimeTzTest: Types/TimeTest: Test insert and select of all datetime (with and without timezones) and time precisions. Schema/VariableDateTimePrecisionIntrospectionTest: Ensure column definitions for created database tables match expected column definitions for all time and datetime types with all precisions.

AbstractMySQLPlatform. Add precision to DATETIME etc columns where column specification includes a scale. No precision is added where no column scale is specified. Add additional cases to AbstractMySQLPlatformTestCase:: testGetTimeTypeDeclarationSql() and testGetDateTimeTypeDeclarationSql() covering high precision times.

Add precision to TIMESTAMP etc columns where column specification includes a scale. Where no scale is specified, default remains TIMESTAMP(0). Test platform specific fallback datetime format strings.

Add precision to DATETIME2, DATETIMEOFFSET and TIME columns where column specification includes a scale. Where no scale is specified, default remains DATETIME2(6), DATETIMEOFFSET(6) or TIME(0), as the case may be. SQLServer supports time precision up to seven decimal places on all time and datetime columns except smalldatetime which doctrine does not use. Since php datetime objects support a maximum precision of only 6, this is the practical limit of precision although columns with a scale of 7 can still be set. Also add platform-specific getFallbackDateTimeTzFormatString() method.

Add precision to TIMESTAMP etc columns where column specification includes a scale. Where no scale is specified, default remains TIMESTAMP(0). DB2 supports precisions of between 0 and 12 for datetime columns but not time columns which are to the nearest second only. Since php datetime objects support a maximum precision of only 6, this is the maximum useful precision although datetime columns with a scale of 12 can still be set. The DB2 schema manager cannot infer a doctrine datetimetz type from a database column as the type lacks a DC2Type comment. They are introspected as datetime columns. The comparator then identifies a difference between doctrine and database schemas for datetimetz types. This error is ignored in the VariableDateTimePrecisionIntrospectionTest as is not due to adding variable precision datetime support. ISSUE WITH SUPPORTING VARIABLE PRECISION TIME DB2 uses TIME columns for TimeTypes and TIME does not support fractional seconds. Switching to TIMESTAMP would require changing the format string to specify a fixed date. This means variable precision time columns cannot be implemented in a backwards compatible way (i.e. without converting all pre-existing TIME columns in the database) because TimeType::convertToDatabaseValue() has no knowledge of the column, the details of which would be needed to choose the correct format string for conversion.

Add precision to TIMESTAMP columns where column specification includes a scale. Where no scale is specified, default remains TIMESTAMP(0) or TIMESTAMP(0) WITH TIME ZONE. Oracle supports from 0-9 fractional seconds for TIMESTAMP columns only. On the Oracle platform, variable precision is therefore only available for DateTimeType and DateTimeTzType and not TimeType. Oracle sets the string format of timestamps in session variables (NLS_TIMESTAMP_FORMAT and NLS_TIMESTAMP_TZ_FORMAT). Changing these settings to support high precision timestamps means Oracle transmits a changed string representation of datetime objects to the application which is a BC break. Using high precision datetimes is therefore an opt-in feature. Add a new platform, OracleHptPlatform, which supports high precision timestamps. Add a boolean driverOption ('high_precision_timestamps') detected by the Oracle drivers which, if the option is true, set a flag used by the AbstractOracleDriver to determine which platform instance to return (the platform supporting high precision timestamps or the legacy platform). The driver option is also detected by the InitializeSession middleware and OracleSessionInit post-connect event handler, in both cases to determine the appropriate NLS_TIMESTAMP_ format strings. This ensures the NLS_TIMESTAMP_ format strings and php datetime format strings set by the platform match. Use '.FF6' as the high precision specifier as it matches php's '.u' format string. The maximum practical time resolution on the Oracle platform is therefore microsecond. Trailing zeroes will be added in the database where a higher precision is specified by the column but these will not be available to doctrine. If the high_precision_timetamps driverOption is set to false or unset, precision can still be added to datetime columns but the highest resolution timestamp that can be represented will remain one second. The schema manager cannot identify a doctrine TimeType from a database column because DATE is used and this maps to DateType without a DC2TYpe comment. This introspection error is ignored in the relevant test as is not due to adding variable precision datetime support. ISSUE WITH SUPPORTING VARIABLE PRECISION ON TIME COLUMNS Oracle uses DATE columns for TimeTypes and DATE does not support fractional seconds. Switching to TIMESTAMP would require changing the format string to specify a fixed date for TimeTypes. This means variable precision time columns cannot be implemented in a backwards compatible way (i.e. without converting all pre-existing DATE columns in the database) because TimeType::convertToDatabaseValue() has no knowledge of the column, the details of which would be needed to choose the correct format string for conversion. ** Also fixes php format strings for datetimes including timezones ** The datetime format Oracle uses for conversion of strings to times is specified in NLS_TIMESTAMP_TZ_FORMAT. It has historically been set to 'YYYY-MM-DD HH24:MI:SS TZH:TZM', i.e. with a space between time and timezone. The php format string was historically set as 'Y-m-d H:i:sP', i.e. without a space between time and timezone. This means that times with negative timezone differences are read from the database incorrectly as times with positive timezone differences. This patch conforms the php format string to the NLS_TIMESTAMP_TZ_FORMAT.

Sqlite has no concept of precision as regards timestamps because they are stored as strings in the database with an arbitrary number of decimal digits. Historically, the dbal has added no decimal digits to times or datetimes on the sqlite platform. Adding decimal digits would not change the string representation in the database of historical data but new data would have decimal digits added even where one second resolution was required. To ensure this inconsistency does not cause issues, high precision times and datetimes are enabled by setting a driverOption called 'high_precision_timestamps' to true. All times and datetimes will then be stored with six decimal digits added.

derrabus · 2023-08-31T08:51:10Z

You PR contains a lot of unrelated commits now. Can you fix that? Otherwise, we cannot review it.

cgknx · 2023-08-31T09:02:50Z

Branch cleaned up now, I hope.

github-actions · 2023-11-30T03:01:28Z

There hasn't been any activity on this pull request in the past 90 days, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 7 days.
If you want to continue working on it, please leave a comment.

jurchiks · 2023-12-02T05:00:44Z

Why has this been seemingly abandoned? This feature has been needed for over 6 years now! All the tests pass, the only thing blocking this is people. @derrabus would you mind?

I just ran into this issue on a new project and it has been driving me insane for hours, and judging by the amount of comments on the original ticket, I am not even remotely the only one.

jurchiks · 2023-12-02T06:00:04Z

src/Platforms/AbstractPlatform.php

+     */
+    protected function getDateTimeTypePrecisionSQLSnippet(array $column): string
+    {
+        $scale = empty($column['scale']) || (int) $column['scale'] === Column::DATETIME_SCALE_NOT_SET


This should use precision, not scale.
Postgres official documentation calls it precision: https://www.postgresql.org/docs/current/datatype-datetime.html#:~:text=time%2C%20timestamp%2C%20and%20interval%20accept%20an%20optional%20precision%20value%20p

Actually, this is weird, I dumped the output of the $column array when generating a diff migration, and for some reason precision is 10 by default even though the code says it should be 0: https://github.com/doctrine/orm/blob/2.17.x/lib/Doctrine/ORM/Mapping/Column.php#L43

^ array:13 [ "name" => "updated_at" "type" => App\Doctrine\DBAL\Types\ChronosDateTimeType^ {#319} "default" => null "notnull" => true "length" => null "precision" => 10 "scale" => 0 "fixed" => false "unsigned" => false "autoincrement" => false "columnDefinition" => null "comment" => "(DC2Type:chronos_datetime)" "version" => false ]

So I guess scale is correct, but in that case all the newly introduced methods that are called get...TypePrecision...() should be renamed to get...TypeScale...().

I'm also noticing that currently all the Type class' convertToPHPValue() methods try to brute-force the conversion instead of just checking the $column['scale'] (which currently isn't passed in as a parameter to these methods).
They're also missing the opposite convertToDatabaseValue() methods, which should be using the same format:

convertToDatabaseValue($value, array $column, AbstractPlatform $platform) -> $value->format($platform->getDate/TimeFormatString($column)); convertToPHPValue($value, array $column, AbstractPlatform $platform) -> DateTimeClass:createFromFormat($platform->getDate/TimeFormatString($column), $value);

where getDate/TimeFormatString() return value changes depending on $column['scale'].

derrabus · 2023-12-02T07:01:34Z

Why has this been seemingly abandoned?

Because people have lives and families and day jobs, basically.

This feature has been needed for over 6 years now!

What's that supposed to tell me? This PR has been in progress since March this year which is when I had time to conduct the thorough review that this contribution needed. It was far from being mergable at that time.

All the tests pass, the only thing blocking this is people. @derrabus would you mind?

Yes, I do mind, actually. I really don't understand where this attitude is coming from. I'm not the only one able to test or review this contribution. I just happened to be the guy who conducted a review on March in his unpaid freetime.

I just ran into this issue on a new project and it has been driving me insane for hours, and judging by the amount of comments on the original ticket, I am not even remotely the only one.

So, you're the ideal volunteer to perform another review and test this feature on a real world project.

jurchiks · 2023-12-02T07:05:36Z

I really don't understand where this attitude is coming from. I'm not the only one able to test or review this contribution.

You are the only reviewer whose comments were pending at the time the PR went stale:

And since your comments were the pending ones, it makes sense that you would check up on them after the PR was updated.

Especially considering that the OP fixed what you asked on the same day you asked.

Because people have lives and families and day jobs, basically.

3 months...

derrabus · 2023-12-02T07:24:19Z

Sorry, but I'm not wasting any more energy on you. If you want to see this merged, make yourself useful. If you think you can bully me into working for free, get lost.

jurchiks · 2023-12-02T08:16:05Z

You asked for changes on this PR. Those changes were made the same day you asked.
Are those changes as you wanted them to be or not?
It's a simple question, and all I'm asking for is a yes or no on the PR.

greg0ire · 2023-12-03T17:39:35Z

And since your comments were the pending ones, it makes sense that you would check up on them after the PR was updated.

@jurchiks I think you have no idea what our inbox looks like. I have many, many notifications every day, to the point that I regularly mark all as read so as not to feel too overwhelmed. For Alexander, who is also a Symfony Core contributor, I guess it is way, way worse. Some of them come from people such as you that are "driven insane" by bugs, and do act insane as a consequence. I think you should calm down, try to understand the many reasons why what you did is very wrong, and then profusely apologize before attempting to ask any more questions.

github-actions · 2024-03-03T03:01:44Z

There hasn't been any activity on this pull request in the past 90 days, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 7 days.
If you want to continue working on it, please leave a comment.

github-actions · 2024-03-11T03:01:30Z

This pull request was closed due to inactivity.

simPod · 2024-03-11T08:20:30Z

@greg0ire @derrabus can we reopen? Where is it stuck? Is it waiting to being reviewed or is something still needed from the author?

derrabus added New Feature Types Type Conversion labels Mar 13, 2023

derrabus changed the title ~~Add support for variable precision TIMEs and DATETIMEs.~~ Add support for variable precision TIMEs and DATETIMEs Mar 13, 2023

derrabus reviewed Mar 13, 2023

View reviewed changes

src/Platforms/AbstractPlatform.php Outdated Show resolved Hide resolved

derrabus reviewed Mar 13, 2023

View reviewed changes

tests/Functional/Types/DateTimeTest.php Outdated Show resolved Hide resolved

derrabus reviewed Mar 13, 2023

View reviewed changes

tests/Functional/Types/DateTimeTest.php Outdated Show resolved Hide resolved

derrabus reviewed Mar 13, 2023

View reviewed changes

tests/Functional/Types/DateTimeTest.php Outdated Show resolved Hide resolved

tests/Functional/Types/DateTimeTest.php Outdated Show resolved Hide resolved

tests/Functional/Types/DateTimeTest.php Outdated Show resolved Hide resolved

morozov reviewed Mar 18, 2023

View reviewed changes

src/Platforms/AbstractPlatform.php Outdated Show resolved Hide resolved

cgknx force-pushed the variable-precision-time branch from 90388b4 to 8b526f9 Compare March 24, 2023 12:20

cgknx requested review from morozov and derrabus and removed request for morozov March 24, 2023 13:09

morozov reviewed Mar 24, 2023

View reviewed changes

morozov reviewed Mar 28, 2023

View reviewed changes

cgknx force-pushed the variable-precision-time branch from 5a17daf to 8e13b19 Compare March 30, 2023 20:23

derrabus requested changes Apr 3, 2023

View reviewed changes

cgknx force-pushed the variable-precision-time branch from e594a88 to 285c8f9 Compare April 11, 2023 10:53

derrabus mentioned this pull request Apr 14, 2023

feat: add support for comparing pg timestamp and time precisions #6007

Closed

simPod reviewed Apr 14, 2023

View reviewed changes

cgknx force-pushed the variable-precision-time branch 2 times, most recently from 503be37 to 93b7500 Compare June 28, 2023 11:33

cgknx added 8 commits August 30, 2023 18:41

tests/Types/Datetime. Test high precision type conversions.

5d20082

Test conversion of datetimes and times including and excluding microseconds in the datetime string to php Datetime objects.

PostgreSQL. Add variable precision time support.

20e29fe

Add precision to TIMESTAMP etc columns where column specification includes a scale. Where no scale is specified, default remains TIMESTAMP(0). Test platform specific fallback datetime format strings.

cgknx force-pushed the variable-precision-time branch from eb41a67 to fe4305f Compare August 31, 2023 09:00

github-actions bot added the Stale label Nov 30, 2023

jurchiks reviewed Dec 2, 2023

View reviewed changes

derrabus changed the base branch from 3.7.x to 3.8.x December 2, 2023 06:42

github-actions bot removed the Stale label Dec 3, 2023

github-actions bot added the Stale label Mar 3, 2024

github-actions bot closed this Mar 11, 2024

derrabus mentioned this pull request May 29, 2024

Add precision to MSSQL datetime2 #6393

Draft

derrabus mentioned this pull request Jul 8, 2024

feat: add TIMESTAMP and TIME precision #6438

Closed

derrabus mentioned this pull request Aug 19, 2024

Migrations always generated for custom type with DBAL 4 doctrine/migrations#1441

Open

		. " NLS_TIMESTAMP_FORMAT = 'YYYY-MM-DD HH24:MI:SS.FF6'"
		. " NLS_TIMESTAMP_TZ_FORMAT = 'YYYY-MM-DD HH24:MI:SS.FF6 TZH:TZM'"

		* Conversion will fail where the format string returned by getDateTimeFormatString() contains a
		* microsecond specifier (.u) but the string to be converted has no decimal digits.

		return $this->_driver instanceof AbstractDriverMiddleware &&
		$this->_driver->wrappedDriverInstanceOf(AbstractOracleDriver::class);

Add support for variable precision TIMEs and DATETIMEs #5961

Add support for variable precision TIMEs and DATETIMEs #5961

Conversation

cgknx commented Mar 9, 2023 • edited Loading

Summary

Use

Notes

SQLite

Oracle

derrabus commented Mar 13, 2023 • edited Loading

cgknx commented Mar 13, 2023

cgknx commented Mar 13, 2023

derrabus commented Mar 13, 2023

cgknx commented Mar 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derrabus Mar 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgknx commented Mar 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

morozov Mar 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgknx commented Mar 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgknx commented Apr 11, 2023

simPod left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derrabus commented Aug 31, 2023

cgknx commented Aug 31, 2023

github-actions bot commented Nov 30, 2023

jurchiks commented Dec 2, 2023 • edited Loading

jurchiks Dec 2, 2023 • edited Loading

Choose a reason for hiding this comment

jurchiks Dec 2, 2023 • edited Loading

Choose a reason for hiding this comment

jurchiks Dec 2, 2023 • edited Loading

Choose a reason for hiding this comment

derrabus commented Dec 2, 2023

jurchiks commented Dec 2, 2023 • edited Loading

derrabus commented Dec 2, 2023

jurchiks commented Dec 2, 2023

greg0ire commented Dec 3, 2023

github-actions bot commented Mar 3, 2024

github-actions bot commented Mar 11, 2024

simPod commented Mar 11, 2024

cgknx commented Mar 9, 2023 •

edited

Loading

derrabus commented Mar 13, 2023 •

edited

Loading

derrabus Mar 31, 2023 •

edited

Loading

morozov Mar 28, 2023 •

edited

Loading

jurchiks commented Dec 2, 2023 •

edited

Loading

jurchiks Dec 2, 2023 •

edited

Loading

jurchiks Dec 2, 2023 •

edited

Loading

jurchiks Dec 2, 2023 •

edited

Loading

jurchiks commented Dec 2, 2023 •

edited

Loading