Skip to content

Ban DateTime from the codebase #47730

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
Merged
merged 1 commit into from
Oct 9, 2022
Merged

Ban DateTime from the codebase #47730

merged 1 commit into from
Oct 9, 2022

Conversation

WebMamba
Copy link
Contributor

@WebMamba WebMamba commented Sep 29, 2022 •
edited
Loading

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!

@carsonbot carsonbot added this to the 6.2 milestone Sep 29, 2022
@WebMamba WebMamba changed the title DateTime to DateTimeimmutable in BrowserKit component Ban DateTime from the codebase Sep 29, 2022
@stof
Copy link
Member

stof commented Sep 29, 2022

you should not mark this PR is fixing the issue, as it only does part of the work. We don't want the issue to get closed automatically by github when only that PR is merged. So having a link to the issue is good, but you should not have Fix before it.

Note: I edited the PR description to remove it, but please be careful about that for your next PRs

stof
stof requested changes Sep 29, 2022
View reviewed changes
Copy link
Member

@stof stof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This component is still using date_create, which needs to be migrated to date_create_immutable instead

@WebMamba
Copy link
Contributor Author

you should not mark this PR is fixing the issue, as it only does part of the work. We don't want the issue to get closed automatically by github when only that PR is merged. So having a link to the issue is good, but you should not have Fix before it.

Note: I edited the PR description to remove it, but please be careful about that for your next PRs

thanks for for it @stof, I wasn't aware of this.

This component is still using date_create, which needs to be migrated to date_create_immutable instead

Nice catch thanks !

stof
stof previously approved these changes Sep 29, 2022
View reviewed changes
@stof
Copy link
Member

stof commented Sep 29, 2022

@WebMamba looks like I miunderstood what you meant by "I'll process component by component", as it seems like you are adding other components in the same PR (which would invalidate my previous approval as it was only for BrowserKit).

If you want us to review it component by component, separate PRs would be a better fit.

@WebMamba
Copy link
Contributor Author

WebMamba commented Sep 29, 2022 •
edited
Loading

@WebMamba looks like I miunderstood what you meant by "I'll process component by component", as it seems like you are adding other components in the same PR (which would invalidate my previous approval as it was only for BrowserKit).

If you want us to review it component by component, separate PRs would be a better fit.

@nicolas-grekas asked me to process it like this. I do all the easy changes here and if we spot a tricky one we do a dedicated PR

@stof stof dismissed their stale review September 29, 2022 15:02

More components are being worked on

@WebMamba WebMamba requested a review from yceruto as a code owner September 30, 2022 08:48
@WebMamba
Copy link
Contributor Author

Thanks, @stof for your review! All the things you spotted are corrected now! 👍

@WebMamba WebMamba requested a review from xabbuh as a code owner October 3, 2022 09:28
@WebMamba WebMamba mentioned this pull request Oct 3, 2022
Closed
@WebMamba WebMamba mentioned this pull request Oct 6, 2022
Closed
@fabpot
Copy link
Member

fabpot commented Oct 9, 2022

Thank you @WebMamba.

@fabpot fabpot merged commit 053613c into symfony:6.2 Oct 9, 2022
@WebMamba WebMamba deleted the ban_date_time_from_the_codebase branch October 20, 2022 14:24
@fabpot fabpot mentioned this pull request Oct 24, 2022
Merged
PhilETaylor pushed a commit to PhilETaylor/symfony that referenced this pull request 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
Reviewers

@stof stof stof requested changes

@OskarStark OskarStark OskarStark left review comments

@fabpot fabpot fabpot approved these changes

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@dunglas dunglas Awaiting requested review from dunglas dunglas is a code owner

@chalasr chalasr Awaiting requested review from chalasr chalasr is a code owner

@yceruto yceruto Awaiting requested review from yceruto yceruto is a code owner

@xabbuh xabbuh Awaiting requested review from xabbuh xabbuh is a code owner

Assignees
No one assigned
Labels
Deprecation Feature Status: Reviewed
Projects
None yet
Milestone
6.2
Development

Successfully merging this pull request may close these issues.
6 participants
@WebMamba @stof @fabpot @nicolas-grekas @OskarStark @carsonbot