Skip to content

[DoctrineBridge] Add new DatePointDateType Doctrine type #60237

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

Open
wants to merge 1 commit into
base: 7.4
Choose a base branch
from

Conversation

wkania
Copy link
Contributor

@wkania wkania commented Apr 17, 2025

Q A
Branch 7.4
Bug fix no
New feature yes
Deprecations no
License MIT

Doctrine provides both date_immutable and datetime_immutable types. Restricting the conversion of DatePoint only to datetime_immutable is problematic.

New version:

Previous pull request.

doctrine:
    dbal:
        types:
            date_point: Symfony\Bridge\Doctrine\Types\DatePointType
            date_point_date: Symfony\Bridge\Doctrine\Types\DatePointDateType
#[ORM\Column(type: 'date_point_date')]
public DatePoint $birthday;

#[ORM\Column(type: 'date_point')]
public DatePoint $createdAt; 

Old version:

Therefore, I propose renaming the current DatePointType to DateTimePointType, and introducing a new DatePointType.

Previous pull request.

If this pull request is to be merged, it should be included in version 7.3 to avoid any breaking changes in the future.

doctrine:
    dbal:
        types:
            date_point: Symfony\Bridge\Doctrine\Types\DatePointType
            datetime_point: Symfony\Bridge\Doctrine\Types\DateTimePointType
#[ORM\Column(type: 'date_point')]
public DatePoint $birthday;

#[ORM\Column(type: 'datetime_point')]
public DatePoint $createdAt;            

@GromNaN
Copy link
Member

GromNaN commented Apr 17, 2025

I removed the direct mentions to avoid notification spam when the PR is merged

@garak
Copy link
Contributor

garak commented Apr 17, 2025

How is it "problematic"?

@wkania
Copy link
Contributor Author

wkania commented Apr 17, 2025

@garak For example, Doctrine Migrations for PostgreSQL will generate migrations using the TIMESTAMP type instead of DATE. The schema will never be fully synchronized if you try to force it. Relational databases have dedicated data types for storing DATE and DATETIME, which is why PHP's DateTime is represented by two separate types in Doctrine.

@derrabus
Copy link
Member

First of all, thank you for bringing up this topic. Secondly: Naming things is hard.

DatePoint is the class that the type maps to, so the name DatePointType is totally accurate for that class. I have no clue what a "date time point" is.

My proposal: Keep the DatePointType as it is and introduce a new DatePointDateType that maps DatePoint objects to DATE columns.

@wkania
Copy link
Contributor Author

wkania commented Apr 17, 2025

@derrabus Ok, so we're leaning more towards Symfony's naming conventions rather than Doctrine's. You contribute a lot to Doctrine, so this is an important note.
Also, I will add a separate entry in the Changelog.

@wkania
Copy link
Contributor Author

wkania commented Apr 18, 2025

@derrabus After the changes you suggested, this is how it will look.

doctrine:
    dbal:
        types:
            date_point: Symfony\Bridge\Doctrine\Types\DatePointType
            date_point_date: Symfony\Bridge\Doctrine\Types\DatePointDateType
#[ORM\Column(type: 'date_point_date')]
public DatePoint $birthday;

#[ORM\Column(type: 'date_point')]
public DatePoint $createdAt; 

@wkania wkania force-pushed the datepointtype_and_datetimepointtype branch from acb035e to 7d6da09 Compare April 18, 2025 17:27
@wkania wkania changed the title [DoctrineBridge] rename DatePointType to DateTimePointType, add new DatePointType [DoctrineBridge] add new DatePointDateType Doctrine type Apr 18, 2025
@wkania wkania force-pushed the datepointtype_and_datetimepointtype branch from 7d6da09 to 31ca901 Compare May 2, 2025 18:22
@wkania wkania requested a review from mtarld May 2, 2025 18:24
@OskarStark OskarStark changed the title [DoctrineBridge] add new DatePointDateType Doctrine type [DoctrineBridge] Add new DatePointDateType Doctrine type May 5, 2025
@wkania wkania force-pushed the datepointtype_and_datetimepointtype branch 2 times, most recently from e5ec79a to 9f41419 Compare May 18, 2025 15:13
@wkania
Copy link
Contributor Author

wkania commented May 18, 2025

Added missing test

@wkania wkania requested a review from OskarStark May 18, 2025 15:15
@fabpot fabpot modified the milestones: 7.3, 7.4 May 26, 2025
@wkania wkania force-pushed the datepointtype_and_datetimepointtype branch from 9f41419 to c18da98 Compare May 29, 2025 19:55
@wkania
Copy link
Contributor Author

wkania commented May 29, 2025

Updated Changelog 7.3 ->7.4.

@wkania wkania requested a review from GromNaN May 29, 2025 20:06
@wkania
Copy link
Contributor Author

wkania commented May 29, 2025

@carsonbot label Feature

@OskarStark
Copy link
Contributor

Just an idea: What about keeping the name add make it configurable instead of having two types?

nicolas-grekas added a commit that referenced this pull request May 30, 2025
…Type` and `TimeType` (wkania)

This PR was merged into the 7.4 branch.

Discussion
----------

[Form] Add `input=date_point` to `DateTimeType`, `DateType` and `TimeType`

| Q             | A
| ------------- | ---
| Branch?       | 7.4
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Issues        |
| License       | MIT

Based on [datetime_immutable](https://symfony.com/blog/new-in-symfony-4-1-added-support-for-immutable-dates-in-forms).

After [DatePointType](#59900) and [DatePointDateType](#60237), it would be great to use Forms without needing to transform values into the DatePoint type manually.

```
use Symfony\Component\Form\Extension\Core\Type\DateType;
use Symfony\Component\Form\Extension\Core\Type\DateTimeType;
use Symfony\Component\Form\Extension\Core\Type\TimeType;
use Symfony\Component\Form\Extension\Core\Type\BirthdayType;

$builder->add('from', DateType::class, [
    'input' => 'date_point',
]);
$builder->add('from', DateTimeType::class, [
    'input' => 'date_point',
]);
$builder->add('from', TimeType::class, [
    'input' => 'date_point',
]);
$builder->add('from', BirthdayType::class, [
    'input' => 'date_point',
]);
```

Alternative: Make symfony/clock a hard requirement and refactor the existing DateTimeImmutableToDateTimeTransformer to return a DatePoint instead. This should not introduce any breaking changes.

Commits
-------

f1160d6 [Form] Add input=date_point to DateTimeType, DateType and TimeType
symfony-splitter pushed a commit to symfony/form that referenced this pull request May 30, 2025
…Type` and `TimeType` (wkania)

This PR was merged into the 7.4 branch.

Discussion
----------

[Form] Add `input=date_point` to `DateTimeType`, `DateType` and `TimeType`

| Q             | A
| ------------- | ---
| Branch?       | 7.4
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Issues        |
| License       | MIT

Based on [datetime_immutable](https://symfony.com/blog/new-in-symfony-4-1-added-support-for-immutable-dates-in-forms).

After [DatePointType](symfony/symfony#59900) and [DatePointDateType](symfony/symfony#60237), it would be great to use Forms without needing to transform values into the DatePoint type manually.

```
use Symfony\Component\Form\Extension\Core\Type\DateType;
use Symfony\Component\Form\Extension\Core\Type\DateTimeType;
use Symfony\Component\Form\Extension\Core\Type\TimeType;
use Symfony\Component\Form\Extension\Core\Type\BirthdayType;

$builder->add('from', DateType::class, [
    'input' => 'date_point',
]);
$builder->add('from', DateTimeType::class, [
    'input' => 'date_point',
]);
$builder->add('from', TimeType::class, [
    'input' => 'date_point',
]);
$builder->add('from', BirthdayType::class, [
    'input' => 'date_point',
]);
```

Alternative: Make symfony/clock a hard requirement and refactor the existing DateTimeImmutableToDateTimeTransformer to return a DatePoint instead. This should not introduce any breaking changes.

Commits
-------

f1160d6617f [Form] Add input=date_point to DateTimeType, DateType and TimeType
@wkania
Copy link
Contributor Author

wkania commented May 30, 2025

@OskarStark
I see several drawbacks to this solution:

  • Doctrine provides two separate classes for this: DateImmutableType and DateTimeImmutableType
    (see: Doctrine DBAL Types).

  • We would need to extend the Type class and implement two interfaces:
    PhpDateTimeMappingType and PhpDateTimeTimeMappingType.

  • If we introduce some kind of parameter to handle formatting, we won’t be able to use it in
    DatePointType , which is where the decision about the database storage format
    should be made. The convertToDatabaseValue method does not allow for that, and it's precisely this method that causes problems in your idea. Each platform (db) has specific format for: getDateFormatString, getDateTimeFormatString and getTimeFormatString.

  • We should also keep in mind the existence of TimeImmutableType, which ideally should have a corresponding type named DatePointTimeType.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants