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

Add ToDateTime parameter attribute #77

Merged
merged 22 commits into from
Apr 3, 2024
Merged

Add ToDateTime parameter attribute #77

merged 22 commits into from
Apr 3, 2024

Conversation

vjik
Copy link
Member

@vjik vjik commented Apr 1, 2024

Q A
Is bugfix?
New feature? ✔️
Breaks BC?

@vjik vjik requested a review from a team April 1, 2024 12:49
Copy link

codecov bot commented Apr 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (c1c28a5) to head (449cb6b).

Additional details and impacted files
@@             Coverage Diff             @@
##              master       #77   +/-   ##
===========================================
  Coverage     100.00%   100.00%           
- Complexity       198       234   +36     
===========================================
  Files             29        31    +2     
  Lines            524       613   +89     
===========================================
+ Hits             524       613   +89     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@vjik vjik added the status:code review The pull request needs review. label Apr 1, 2024
Copy link
Member

@xepozz xepozz left a comment

Choose a reason for hiding this comment

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

As I can see there is not a lot of modification to make the new attribute be able to convert a value to DateTime or DateTimeImmutable objects depends on the property type.

Could you make the new attribute to be able to do that?
I'd rename it ToDateTime or create a separate attribute.

docs/guide/en/typecasting.md Outdated Show resolved Hide resolved
docs/guide/ru/typecasting.md Outdated Show resolved Hide resolved
tests/Attribute/Parameter/ToDateTimeImmutableTest.php Outdated Show resolved Hide resolved
tests/Attribute/Parameter/ToDateTimeImmutableTest.php Outdated Show resolved Hide resolved
@vjik
Copy link
Member Author

vjik commented Apr 1, 2024

Could you make the new attribute to be able to do that? I'd rename it ToDateTime or create a separate attribute.

PHP recommends use DateTimeImmutable by default. Is there a case for use mutable DateTime?

@xepozz
Copy link
Member

xepozz commented Apr 1, 2024

Could you make the new attribute to be able to do that? I'd rename it ToDateTime or create a separate attribute.

PHP recommends use DateTimeImmutable by default. Is there a case for use mutable DateTime?

Despite on recommendations people still use mutable objects

@samdark
Copy link
Member

samdark commented Apr 1, 2024

I'd go with immutable and add mutable if/when requested.

@vjik
Copy link
Member Author

vjik commented Apr 2, 2024

@xepozz @samdark I added DateTime support, but used immutable object, if it avaliable.

See tests: https://github.com/yiisoft/hydrator/pull/77/files#diff-1d0f0e934fc4f20ba9d5353e9be78c218f5aedd536e243089f516d46221d6c44R242

@vjik vjik requested review from xepozz and samdark April 2, 2024 07:00
@vjik vjik changed the title Add ToDateTimeImmutable parameter attribute Add ToDateTim parameter attribute Apr 2, 2024
@vjik vjik changed the title Add ToDateTim parameter attribute Add ToDateTime parameter attribute Apr 2, 2024
@vjik vjik merged commit c51e692 into master Apr 3, 2024
19 checks passed
@vjik vjik deleted the to-date-time-immutable branch April 3, 2024 05:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status:code review The pull request needs review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants