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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Check dates against DateTimeInterface instead of DateTime #2239

Merged
merged 3 commits into from Apr 30, 2021
Merged

Check dates against DateTimeInterface instead of DateTime #2239

merged 3 commits into from Apr 30, 2021

Conversation

jeromegamez
Copy link
Contributor

Greeting! 馃憢

I noticed that where() queries in which I used DateTimeImmutable objects returned empty results.

This change would allow the usage of DateTimeImmutable or CarbonImmutable in addition to DateTime/Carbon 馃

:octocat:

@jeromegamez jeromegamez changed the title Check dates against DateTimeInterface instead of DateTime in the Query Builder Check dates against DateTimeInterface instead of DateTime Apr 29, 2021
@Smolevich Smolevich self-requested a review April 29, 2021 06:35
@Smolevich Smolevich self-assigned this Apr 29, 2021
@Smolevich Smolevich assigned jeromegamez and unassigned Smolevich Apr 29, 2021
Smolevich
Smolevich reviewed Apr 29, 2021
View reviewed changes
Copy link
Contributor

@Smolevich Smolevich left a comment

Choose a reason for hiding this comment

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

Needs work

tests/QueryBuilderTest.php Outdated Show resolved Hide resolved
@jeromegamez jeromegamez removed their assignment Apr 29, 2021
@divine divine requested a review from Smolevich April 29, 2021 18:43
@codecov-commenter
Copy link

codecov-commenter commented Apr 30, 2021 •
edited

Codecov Report

Merging #2239 (9815f38) into master (73b6a91) will increase coverage by 0.06%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2239      +/-   ##
============================================
+ Coverage     86.82%   86.88%   +0.06%     
  Complexity      664      664              
============================================
  Files            33       33              
  Lines          1556     1556              
============================================
+ Hits           1351     1352       +1     
+ Misses          205      204       -1
Impacted Files Coverage 螖 Complexity 螖
src/Jenssegers/Mongodb/Eloquent/Model.php 93.63% <100.00%> (酶) 83.00 <0.00> (酶)
src/Jenssegers/Mongodb/Query/Builder.php 89.18% <100.00%> (+0.26%) 162.00 <0.00> (酶)

Continue to review full report at Codecov.

Legend - Click here to learn more
螖 = absolute <relative> (impact), 酶 = not affected, ? = missing data
Powered by Codecov. Last update 73b6a91...9815f38. Read the comment docs.

@jeromegamez
Copy link
Contributor Author

Shall I address the StyleCI issues in a separate commit?

@divine
Copy link
Contributor

divine commented Apr 30, 2021

Shall I address the StyleCI issues in a separate commit?

Hello,

No, we'll take care of it later.

Thanks!

@Smolevich Smolevich merged commit 683749c into mongodb:master Apr 30, 2021
@divine
Copy link
Contributor

divine commented Apr 30, 2021

Thank you for your contribution @jeromegamez !

@jeromegamez
Copy link
Contributor Author

馃殌

Thank you both for accepting it! 馃

@jeromegamez jeromegamez deleted the date-time-interface branch April 30, 2021 21:55
@jeromegamez jeromegamez mentioned this pull request Apr 30, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Smolevich Smolevich Smolevich approved these changes

@divine divine divine approved these changes

Assignees
No one assigned
Labels
candidate-for-release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@jeromegamez @codecov-commenter @divine @Smolevich