Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 7 commits into
base: 4.3
from

Conversation

Projects
None yet
5 participants
@roukmoute
Copy link
Contributor

commented Jul 9, 2019

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.

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

This comment has been minimized.

Copy link
@stof

stof Jul 9, 2019

Member

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)

This comment has been minimized.

Copy link
@roukmoute

roukmoute Jul 10, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@stof

stof Jul 10, 2019

Member

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

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

This comment has been minimized.

Copy link
@stof

stof Jul 9, 2019

Member

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

This comment has been minimized.

Copy link
@roukmoute

roukmoute Jul 9, 2019

Author Contributor

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

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

This comment has been minimized.

Copy link
@derrabus

derrabus Jul 9, 2019

Contributor

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

This comment has been minimized.

Copy link
@roukmoute

roukmoute Jul 10, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jul 10, 2019

Member

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

This comment has been minimized.

Copy link
@stof

stof Jul 10, 2019

Member

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

This comment has been minimized.

Copy link
@stof

stof Jul 10, 2019

Member

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 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 nicolas-grekas added this to the 4.3 milestone Jul 10, 2019

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

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jul 10, 2019

Member

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

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

This comment has been minimized.

Copy link
@stof

stof Jul 10, 2019

Member

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.

}
/**
* @throws DBALException

This comment has been minimized.

Copy link
@stof

stof Jul 10, 2019

Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.