[Messenger] use database platform to convert correctly the DateTime #32456

roukmoute · 2019-07-09T09:18:31Z

Q	A
Branch?	4.3
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	yes
Tests pass?	yes
Fixed tickets	#32427
License	MIT

In Doctrine Messenger the method \Symfony\Component\Messenger\Transport\Doctrine\Connection::formatDateTime() is used to format dateTime into this: Y-m-d\TH:i:s.
But this is not supported in all databases platform.

Here we use the database platform to convert correctly the dateTime.

stof · 2019-07-09T10:11:02Z

src/Symfony/Component/Messenger/Transport/Doctrine/Connection.php

-            ':created_at' => self::formatDateTime($now),
-            ':available_at' => self::formatDateTime($availableAt),
+            ':created_at' => $this->convertDateTime($now),
+            ':available_at' => $this->convertDateTime($availableAt),


We could even use the datetime directly as parameter and pass the Doctrine DBAL types (in the third argument) for these, so that DBAL handles the conversion calls for us (and would avoid the need to create a public convertDateTime too, as tests can do the same)

Sorry I don't understand.
Because in this file, the executeQuery method is not the same method of https://github.com/doctrine/dbal/blob/66f28111d57fdc3508d4c97989257291702176cf/lib/Doctrine/DBAL/Connection.php#L883.

it is the same. The driverConnection propert contains a Doctrine\DBAL\Connection, which is exactly the code I linked.

I think @stof means we could use \Doctrine\DBAL\Connection::executeQuery which has the types as parameter.

stof · 2019-07-09T10:11:54Z

src/Symfony/Component/Messenger/Transport/Doctrine/Connection.php

+     *
+     * @deprecated since Symfony 4.4, use convertDateTime instead
+     */
+    public static function formatDateTime(\DateTimeInterface $dateTime, AbstractPlatform $platform)


adding a required argument is a BC break, which is a bad idea for a method kept as a BC layer...

Oh yes, I will add a new commit to fix this.

derrabus · 2019-07-09T21:08:42Z

src/Symfony/Component/Messenger/Transport/Doctrine/Connection.php

+     *
+     * @deprecated since Symfony 4.4, use convertDateTime instead
+     */
+    public static function formatDateTime(\DateTimeInterface $dateTime, AbstractPlatform $platform = null)


You've added a second parameter to the method, but I can find any calls with that parameter being set.

See my commits in order and you will understand my logic :)

that's a BC break, can't be done

it is not a question of commits in order. The existing method that you want to deprecate must not change signature.

you should remove the new argument and always use $dateTime->format('Y-m-d\TH:i:s') in this deprecated method. Your current change still requires changing the caller to add the platform to benefit from the fix, at which point it is as easy to switch to the new method instead.

nicolas-grekas · 2019-07-10T08:43:08Z

src/Symfony/Component/Messenger/Transport/Doctrine/Connection.php

+    /**
+     * @throws DBALException
+     *
+     * @deprecated since Symfony 4.4, use convertDateTime instead


the deprecation must be done in a separate PR, targetting 4.4 (this PR is for 4.3)

stof · 2019-07-10T09:06:33Z

src/Symfony/Component/Messenger/Transport/Doctrine/Connection.php

+     *
+     * @deprecated since Symfony 4.4, use convertDateTime instead
+     */
+    public static function formatDateTime(\DateTimeInterface $dateTime, AbstractPlatform $platform = null)


you should remove the new argument and always use $dateTime->format('Y-m-d\TH:i:s') in this deprecated method. Your current change still requires changing the caller to add the platform to benefit from the fix, at which point it is as easy to switch to the new method instead.

stof · 2019-07-10T09:07:09Z

src/Symfony/Component/Messenger/Transport/Doctrine/Connection.php

+    }
+
+    /**
+     * @throws DBALException


should be removed. We don't care about this one here (and anyway, this API should not be public, so this would be useless)

Tobion

#32456 (comment)
@roukmoute do you have time to finish the PR?

roukmoute · 2019-07-26T13:04:20Z

yep, I've finally received my laptop today so I can fix it this week-end

src/Symfony/Component/Messenger/composer.json

src/Symfony/Component/Messenger/Transport/Doctrine/Connection.php

roukmoute · 2019-08-02T05:10:17Z

@Tobion Any news?

Tobion · 2019-08-04T19:42:46Z

src/Symfony/Component/Messenger/Transport/Doctrine/Connection.php

+
+    private function getDateTimeType()
+    {
+        if (!class_exists('\Doctrine\DBAL\Types\Types')) {


please use \Doctrine\DBAL\Types\Types::class

src/Symfony/Component/Messenger/Transport/Doctrine/Connection.php

Tobion · 2019-08-04T19:48:35Z

src/Symfony/Component/Messenger/Transport/Doctrine/Connection.php

-                ':redeliver_limit' => self::formatDateTime($redeliverLimit),
+                ':redeliver_limit' => $redeliverLimit,
+            ], [
+                ':now' => Type::getType($this->getDateTimeType()),


do you really need to pass the type instance here with Type::getType? I don't think so. Passing the constant value should be enough.

Tobion · 2019-08-04T19:53:31Z

src/Symfony/Component/Messenger/Transport/Doctrine/Connection.php

@@ -281,22 +290,17 @@ private function createQueryBuilder(): QueryBuilder
            ->from($this->configuration['table_name'], 'm');
    }

-    private function executeQuery(string $sql, array $parameters = [])
+    private function executeQuery(string $sql, array $parameters = [], ?array $types = [])


why does $types need to be nullable?

Without that, few tests does not passes.

1) Symfony\Component\Messenger\Tests\Transport\Doctrine\ConnectionTest::testGetAMessageWillChangeItsStatus TypeError: Argument 3 passed to Symfony\Component\Messenger\Transport\Doctrine\Connection::executeQuery() must be of the type array, null given, called in /home/roukmoute/workspace/symfony/src/Symfony/Component/Messenger/Transport/Doctrine/Connection.php on line 150

Would you prefer I change theses tests to pass without this nullable?

The mocks probably need to be fixed so it's does not return a null which violates the contract.

Tobion · 2019-08-29T14:36:02Z

src/Symfony/Component/Messenger/Transport/Doctrine/Connection.php

@@ -306,20 +314,20 @@ private function getSchema(): Schema
    {
        $schema = new Schema([], [], $this->driverConnection->getSchemaManager()->createSchemaConfig());
        $table = $schema->createTable($this->configuration['table_name']);
-        $table->addColumn('id', Type::BIGINT)
+        $table->addColumn('id', Types::BIGINT)


you cannot require Types as it's not released in dbal yet. please just keep using Type everywhere. we can handle the deprecation of it in the future when it's actually released.

You're right!
Would like a method equivalent with checking if the class exist?

only use Type in this PR please. feel free to create a second PR with a solution for the future deprecation

It's still not consistent. Here you implemented the fallback

symfony/src/Symfony/Component/Messenger/Transport/Doctrine/Connection.php

Lines 346 to 352 in 1e04345

private function getDateTimeType()

{

if (!class_exists(\Doctrine\DBAL\Types\Types::class)) {

return Type::DATETIME;

}

return \Doctrine\DBAL\Types\Types::DATETIME_MUTABLE;

but

symfony/src/Symfony/Component/Messenger/Transport/Doctrine/Connection.php

Line 325 in 1e04345

$table->addColumn('created_at', Type::DATETIME)

does not use it. Either use the getDateTimeType helper everywhere, or don't add it at all.

I asked to not use the new Types class because in order to fully fix the deprcation of Type, you would also need to add the fallback of the other types like "text" in

symfony/src/Symfony/Component/Messenger/Transport/Doctrine/Connection.php

Line 321 in 1e04345

$table->addColumn('headers', Type::TEXT)

which you did not. So it's not complete and irrelevant to this PR.

Tobion · 2019-09-30T14:39:57Z

src/Symfony/Component/Messenger/Transport/Doctrine/Connection.php

@@ -108,7 +108,7 @@ public function send(string $body, array $headers, int $delay = 0): string
        $now = new \DateTime();
        $availableAt = (clone $now)->modify(sprintf('+%d seconds', $delay / 1000));

-        $queryBuilder = $this->driverConnection->createQueryBuilder()
+        $queryBuilder = $this->createQueryBuilder()


Why changing this? It's doing an insert, so the helper method seems useless here.

Tobion · 2019-09-30T14:54:43Z

src/Symfony/Component/Messenger/Transport/Doctrine/Connection.php

+            null,
+            null,
+            null,
+            Type::getType($this->getDateTimeType()),


you don't need to call Type::getType. it should be enough to pass the string constant like it's done in other places

symfony/src/Symfony/Component/Messenger/Transport/Doctrine/Connection.php

Line 285 in 1e04345

$this->getDateTimeType(),

stof · 2019-09-30T15:02:24Z

src/Symfony/Component/Messenger/Transport/Doctrine/Connection.php

+            return Type::DATETIME;
+        }
+
+        return \Doctrine\DBAL\Types\Types::DATETIME_MUTABLE;


please add a use statement

roukmoute · 2019-10-18T15:35:49Z

Ping @Tobion @stof

Tobion · 2019-10-23T15:02:23Z

src/Symfony/Component/Messenger/Transport/Doctrine/Connection.php

@@ -280,12 +292,11 @@ private function createQueryBuilder(): QueryBuilder
            ->from($this->configuration['table_name'], 'm');
    }

-    private function executeQuery(string $sql, array $parameters = [])
+    private function executeQuery(string $sql, array $parameters = [], array $types = [])
    {
        $stmt = null;


not needed anymore

Are you sure?

I commented on the line $stmt = null;. The initialization is not needed anymore because it's always set.

Tobion · 2019-10-23T15:04:54Z

src/Symfony/Component/Messenger/Transport/Doctrine/Connection.php

+            null,
+            null,
+            null,
+            Type::getType(Type::DATETIME),


In #32456 (comment) I asked to only pass the constant. No need to call Type::getType anywhere. Please it's very painful to review this PR over and over and the same problems keep being there.

Tobion

Please squash all commits to a single commit. Our tool cannot do it because of different commiters.

Tobion · 2019-10-25T10:57:42Z

... and one more bug fixed, thank you @roukmoute.

… DateTime (roukmoute) This PR was merged into the 4.3 branch. Discussion ---------- [Messenger] use database platform to convert correctly the DateTime | Q | A | ------------- | --- | Branch? | 4.3 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | yes  | Tests pass? | yes | Fixed tickets | #32427 | License | MIT In Doctrine Messenger the method `\Symfony\Component\Messenger\Transport\Doctrine\Connection::formatDateTime()` is used to format dateTime into this: `Y-m-d\TH:i:s`. But this is not supported in all databases platform. Here we use the database platform to convert correctly the dateTime. Commits ------- cfa1156 Format DateTime depending on database platform

roukmoute · 2019-10-25T14:25:32Z

Thanks for all your time @Tobion

carsonbot added Status: Needs Review Messenger Bug Deprecation labels Jul 9, 2019

stof reviewed Jul 9, 2019

View reviewed changes

derrabus requested changes Jul 9, 2019

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels Jul 9, 2019

nicolas-grekas changed the title ~~[Messenger] - use database platform to convert correctly the DateTime~~ [Messenger] use database platform to convert correctly the DateTime Jul 10, 2019

nicolas-grekas added this to the 4.3 milestone Jul 10, 2019

nicolas-grekas reviewed Jul 10, 2019

View reviewed changes

stof reviewed Jul 10, 2019

View reviewed changes

Tobion suggested changes Jul 26, 2019

View reviewed changes

roukmoute requested a review from sroze as a code owner July 28, 2019 09:51

carsonbot added Status: Needs Review and removed Status: Needs Work labels Jul 28, 2019

Tobion reviewed Jul 28, 2019

View reviewed changes

src/Symfony/Component/Messenger/composer.json Outdated Show resolved Hide resolved

Tobion reviewed Jul 28, 2019

View reviewed changes

src/Symfony/Component/Messenger/Transport/Doctrine/Connection.php Outdated Show resolved Hide resolved

Tobion mentioned this pull request Jul 28, 2019

[Messenger] Fix DateTime format on the Doctrine transport #32640

Closed

Tobion reviewed Jul 28, 2019

View reviewed changes

src/Symfony/Component/Messenger/Transport/Doctrine/Connection.php Outdated Show resolved Hide resolved

Tobion reviewed Aug 4, 2019

View reviewed changes

src/Symfony/Component/Messenger/Transport/Doctrine/Connection.php Outdated Show resolved Hide resolved

Tobion reviewed Aug 4, 2019

View reviewed changes

Tobion reviewed Aug 29, 2019

View reviewed changes

Tobion reviewed Sep 30, 2019

View reviewed changes

stof requested changes Sep 30, 2019

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels Sep 30, 2019

Tobion suggested changes Oct 23, 2019

View reviewed changes

Tobion approved these changes Oct 24, 2019

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Work labels Oct 24, 2019

Tobion suggested changes Oct 24, 2019

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Reviewed labels Oct 24, 2019

Format DateTime depending on database platform

cfa1156

Tobion approved these changes Oct 25, 2019

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Work labels Oct 25, 2019

Tobion merged commit cfa1156 into symfony:4.3 Oct 25, 2019

roukmoute deleted the 4.3 branch October 25, 2019 14:25

roukmoute mentioned this pull request Oct 25, 2019

[Messenger] - formatDateTime is not correct in oracle #32427

Closed

fabpot mentioned this pull request Nov 1, 2019

Release v4.3.6 #34209

Merged

	private function getDateTimeType()
	{
	if (!class_exists(\Doctrine\DBAL\Types\Types::class)) {
	return Type::DATETIME;
	}

	return \Doctrine\DBAL\Types\Types::DATETIME_MUTABLE;

[Messenger] use database platform to convert correctly the DateTime #32456

[Messenger] use database platform to convert correctly the DateTime #32456

Conversation

roukmoute commented Jul 9, 2019

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tobion left a comment

Choose a reason for hiding this comment

roukmoute commented Jul 26, 2019 • edited Loading

roukmoute commented Aug 2, 2019

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

Choose a reason for hiding this comment

roukmoute commented Oct 18, 2019

Tobion Oct 23, 2019 • 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

Tobion left a comment

Choose a reason for hiding this comment

Tobion commented Oct 25, 2019

roukmoute commented Oct 25, 2019

roukmoute commented Jul 26, 2019 •

edited

Loading

Tobion Oct 23, 2019 •

edited

Loading