8000 Ban `DateTime` from the codebase · Issue #47580 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

Ban DateTime from the codebase #47580

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.

Sign up for GitHub

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

Jump to bottom
Closed
nicolas-grekas opened this issue Sep 15, 2022 · 12 comments
Closed

Ban DateTime from the codebase #47580

nicolas-grekas opened this issue Sep 15, 2022 · 12 comments
Labels
Help wanted Issues and PRs which are looking for volunteers to complete them. Keep open

Comments

@nicolas-grekas
Copy link
Member
nicolas-grekas commented Sep 15, 2022 •
edited
Loading

By being mutable, DateTime is flawed. We should figure out a way to remove it from the codebase and have some CI check that ensures we don't add it back by mistake. I suppose we could configure psalm to do that check for us.

Help welcome!
@nicolas-grekas nicolas-grekas added the Help wanted Issues and PRs which are looking for volunteers to complete them. label Sep 15, 2022
@stof
Copy link
Member
stof commented Sep 15, 2022

We will probably not be able to ban it entirely. We still need to have support for working with DateTime in several places (for instance, the form and serializer components need to support them, otherwise we're not banning them from the core but from the ecosystem)

@nicolas-grekas
Copy link
Member Author

True. We need to keep supporting them but ideally we would not generate any anymore in v7.
We could also try to actively move ppl out of DateTime by deprecating handling them in some critical places, but that's another step.

@ro0NL
Copy link
Contributor
ro0NL commented Sep 15, 2022

i'll assume we generally don't care about tests?

@nicolas-grekas
Copy link
Member Author

We do, the ban should be on all the codebase.

@ro0NL
Copy link
Contributor
ro0NL commented Sep 15, 2022

What's your thought on DateTimeInterface? Do we prefer DateTime|DateTimeImmutable where needed?

@nicolas-grekas
Copy link
Member Author
nicolas-grekas commented Sep 15, 2022 •
edited
Loading

We should use DateTimeImmutable when possible, and consider BC/FC otherwise to use DateTimeInterface or not.
This might mean in practice: use DateTimeInterface on arguments and DateTimeImmutable on return types.
Ideally, the engine should consider the union as equivalent to the interface. That's not the case right now: https://3v4l.org/vBTvD

@stof
Copy link
Member
stof commented Sep 15, 2022

To me, when we accept an input for a date we will format, we should use DateTimeInterface.
For return types (and for argument types where it makes sense, for instance in private methods that might care about that), we should use DateTimeImmutable rather than the union type)

@ghost
Copy link
ghost commented Sep 21, 2022 •
edited by ghost
Loading

This is HUGE! Mutable DateTime should have been reverted from the PHP Core already in 5.5 when DateTimeImmutable was introduced to address DateTime's mutability issues. Unfortunately this did not happen and we still have it around today.
I think if Symfony makes such a big step in their codebase then this will speed up the process to remove it from the core what I think has already started with Derick's tweet in July.
I would like to help you with the migration.

This was referenced Sep 29, 2022
Merged
Closed
@ro0NL
Copy link
Contributor
ro0NL commented Oct 4, 2022 •
edited
Loading

DateTime is flawed.

what's PHP internals POV on the matter? I feel like preserving DateTime with new immutable methods is the fastest route, while deprecating anything mutable.

if DateTime is flawed, so is DateTimeInterface IMHO

@WebMamba WebMamba mentioned this issue Oct 6, 2022
Closed
fabpot added a commit that referenced this issue Oct 9, 2022
@fabpot
feature #47730 Ban DateTime from the codebase (WebMamba)
053613c 
This PR was squashed before being merged into the 6.2 branch.

Discussion
----------

Ban DateTime from the codebase

| Q             | A
| ------------- | ---
| Branch?       | 6.2
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | yes
| Tickets       | #47580
| License       | MIT
| Doc PR        | symfony

As discuss in this issue, the purpose of this PR is to remove DateTime from the code base in favor to DateTimeImmutable. I will process it component by component. Feel free to discuss!

Commits
-------

689385a Ban DateTime from the codebase
@WebMamba WebMamba mentioned this issue Oct 20, 2022
Merged
fabpot added a commit that referenced this issue Oct 20, 2022
@fabpot
minor #47937 [FrameworkBundle] remove DateTime from core Bundles (Web…
823f332 
…Mamba)

This PR was merged into the 6.2 branch.

Discussion
----------

[FrameworkBundle] remove DateTime from core Bundles

| Q             | A
| ------------- | ---
| Branch?       | 6.2
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Tickets       | #47580
| License       | MIT
| Doc PR        | symfony/symfony-docs

Remove DateTime from all core bundles. Find also some DateTime in the SecurityBundle, but expose to BC breaks so will be treated in another PR.

Commits
-------

03ea8ff remove DateTime from FrameworkBundle
Marion-Valls added a commit to Marion-Valls/symfony that referenced this issue Jan 14, 2023
@Marion-Valls
minor symfony#47580 [Serializer] Remove DateTime object from Serializer
0b0c442
Marion-Valls added a commit to Marion-Valls/symfony that referenced this issue Jan 14, 2023
@Marion-Valls
minor symfony#47580 [Serializer] Update Test
d23d4c7
Marion-Valls added a commit to Marion-Valls/symfony that referenced this issue Jan 14, 2023
@Marion-Valls
minor symfony#47580 [Serializer] Fix some tests
4385d42
@Marion-Valls Marion-Valls mentioned this issue Jan 14, 2023
Closed
@carsonbot
Copy link

Thank you for this issue.
There has not been a lot of activity here for a while. Has this been resolved?

@nesl247
Copy link
nesl247 commented May 3, 2023

It has not been resolved.

@carsonbot carsonbot removed the Stalled label May 3, 2023
This was referenced May 10, 2023
Merged
Merged
nicolas-grekas added a commit that referenced this issue May 10, 2023
@nicolas-grekas
minor #50289 [Messenger] Use immutable dates in the storage when usin…
fb234d5 
…g Doctrine (nicolas-grekas)

This PR was merged into the 6.3 branch.

Discussion
----------

[Messenger] Use immutable dates in the storage when using Doctrine

| Q             | A
| ------------- | ---
| Branch?       | 6.3
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Tickets       | Part of #47580
| License       | MIT
| Doc PR        | -

We already started using `DateTimeImmutable` in that class but we missed these updates.

Commits
-------

d6563a1 [Messenger] Use immutable dates in the storage when using Doctrine
@nicolas-grekas nicolas-grekas mentioned this issue May 11, 2023
Merged
fabpot added a commit that referenced this issue Jun 3, 2023
@fabpot
minor #50297 Remove unnecessary usages of DateTime (nicolas-grekas)
044b344 
This PR was merged into the 6.4 branch.

Discussion
----------

Remove unnecessary usages of DateTime

| Q             | A
| ------------- | ---
| Branch?       | 6.3
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Tickets       | Part of #47580
| License       | MIT
| Doc PR        | -

Together with #50290, this PR removes `DateTime` everywhere possible.

What remains is:
- test cases that test both DateTime and DateTimeImmutable - legit to keep
- date/time form-types and transformers in the Form component - we'd need a separate effort for them, related to #50295
- `PersistentTokenInterface::getLastUsed(): \DateTime` - separate effort also

Commits
-------

8b08294 Remove unnecessary usages of DateTime
fabpot added a commit that referenced this issue Jul 13, 2023
@fabpot
feature #50290 [Security] Make PersistentToken immutable and tell `…
31bad80 
…TokenProviderInterface::updateToken()` implementations should accept `DateTimeInterface` (nicolas-grekas)

This PR was merged into the 6.4 branch.

Discussion
----------

[Security] Make `PersistentToken` immutable and tell `TokenProviderInterface::updateToken()` implementations should accept `DateTimeInterface`

| Q             | A
| ------------- | ---
| Branch?       | 6.4
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Tickets       | Part of #47580
| License       | MIT
| Doc PR        | -

Commits
-------

3df345c [Security] Make `PersistentToken` immutable and tell `TokenProviderInterface::updateToken()` implementations should accept `DateTimeInterface`
@nicolas-grekas
Copy link
Member Author

Some occurrences remain but they're impossible to remove IIUC (until someone proves me wrong - please do ;) )

PhilETaylor pushed a commit to PhilETaylor/symfony that referenced this issue Sep 6, 2023
@fabpot
feature symfony#47730 Ban DateTime from the codebase (WebMamba)
5bbee25 
This PR was squashed before being merged into the 6.2 branch.

Discussion
----------

Ban DateTime from the codebase

| Q             | A
| ------------- | ---
| Branch?       | 6.2
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | yes
| Tickets       | symfony#47580
| License       | MIT
| Doc PR        | symfony

As discuss in this issue, the purpose of this PR is to remove DateTime from the code base in favor to DateTimeImmutable. I will process it component by component. Feel free to discuss!

Commits
-------

689385a Ban DateTime from the codebase
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Help wanted Issues and PRs which are looking for volunteers to complete them. Keep open
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@nicolas-grekas @stof @nesl247 @ro0NL @derrabus @carsonbot
0