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

Infer datetime_immutable DBAL type for \DateTimeImmutable instance parameters #8328

Merged
merged 2 commits into from
Dec 5, 2020
Merged

Infer datetime_immutable DBAL type for \DateTimeImmutable instance parameters #8328

merged 2 commits into from
Dec 5, 2020

Conversation

vhenzl
Copy link
Contributor

@vhenzl vhenzl commented Nov 5, 2020

The support for passing \DateTimeImmutable instance as a query parameter has been added to ORM in #1333 (the year 2015), a long time before immutable date types (datetime_immutable etc) were introduced to DBAL in doctrine/dbal#2450 (2017).

Back then, it made sense to treat \DateTimeImmutable (or any \DateTimeInterface) in the same way as \DateTime and infer parameter type as datetime. However, when immutable date types were later added to DBAL, it wasn't reflected anyhow in type inference in ORM and \DateTimeImmmutable instances are still inferred as datetime DBAL type.

This PR fixes this IMO incorrect behaviour of ParameterTypeInferer::inferType(): for a \DateTimeImmmutable parameter, it now returns datetime_immutable DBAL type; for \DateTime or any other types implementing \DateTimeInterface, it returns datetime DBAL type as it did before.

This behaviour is in line with DateTimeImmutableType handling only \DateTimeImmutable and DateTimeType handling any \DateTimeInterface.

Why? In most cases, it doesn't matter and datetime works for \DateTimeImmutable parameters just fine. But it does matter if using custom implementation of datetime_immutable type like UTCDateTimeImmutableType from simpod/doctrine-utcdatetime. Then the broken type inference is revealed.

This is partially related to #6443, however, this PR isn't about custom DBAL types but about correct type inference for build-in types.

@vhenzl vhenzl requested a review from SenseException November 8, 2020 07:09
SenseException
SenseException previously approved these changes Nov 8, 2020
@vhenzl vhenzl changed the base branch from 2.7 to 2.8.x December 5, 2020 06:39
@vhenzl
Copy link
Contributor Author

vhenzl commented Dec 5, 2020

Rebased and target changed to 2.8.x branch now that v2.8 has been released.

@greg0ire greg0ire added the Bug label Dec 5, 2020
@greg0ire greg0ire merged commit e7d33eb into doctrine:2.8.x Dec 5, 2020
@greg0ire greg0ire added this to the 2.8.2 milestone Dec 5, 2020
@greg0ire
Copy link
Member

greg0ire commented Dec 5, 2020

Thanks @vhenzl !

@vhenzl vhenzl deleted the infer-datetimeimmutable branch December 5, 2020 22:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants