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 doesntExist to passthru #2194

Merged
merged 1 commit into from Apr 30, 2021
Merged

Add doesntExist to passthru #2194

merged 1 commit into from Apr 30, 2021

Conversation

simonschaufi
Copy link
Contributor

@simonschaufi simonschaufi commented Feb 2, 2021

Those calls should return the same result but instead the call to "doesntExist" returns an object.

!$user->groups()->exists();
$user->groups()->doesntExist();

closes #1765

Copy link
Contributor

@divine divine left a comment

Choose a reason for hiding this comment

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

Hello,

There are other missing passthru functions. See #1765

Could you please kindly add missing ones from https://github.com/illuminate/database/blob/8.x/Eloquent/Builder.php#L81-L84 to make it consistent?

I have prepared my own PR for this a few months back but it was delayed as we didn't decide how to test this at all between versions.

Thanks!

@divine
Copy link
Contributor

divine commented Feb 23, 2021

Hello @simonschaufi,

Do you plan to complete this PR or should I close and push on my own?

Thanks!

@simonschaufi
Copy link
Contributor Author

simonschaufi commented Feb 23, 2021

@divine you can make adjustments on my branch actually. I'm traveling at the moment and could finish it in April

@codecov-io
Copy link

codecov-io commented Feb 25, 2021

Codecov Report

Merging #2194 (d061417) into master (09fcda8) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #2194   +/-   ##
=========================================
  Coverage     86.93%   86.93%           
  Complexity      663      663           
=========================================
  Files            33       33           
  Lines          1554     1554           
=========================================
  Hits           1351     1351           
  Misses          203      203           
Impacted Files Coverage Δ Complexity Δ
src/Jenssegers/Mongodb/Eloquent/Builder.php 87.93% <ø> (ø) 22.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 09fcda8...d061417. Read the comment docs.

@codecov-commenter
Copy link

codecov-commenter commented Apr 29, 2021

Codecov Report

Merging #2194 (b54db68) into master (09fcda8) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #2194   +/-   ##
=========================================
  Coverage     86.93%   86.93%           
  Complexity      663      663           
=========================================
  Files            33       33           
  Lines          1554     1554           
=========================================
  Hits           1351     1351           
  Misses          203      203           
Impacted Files Coverage Δ Complexity Δ
src/Jenssegers/Mongodb/Eloquent/Builder.php 87.93% <ø> (ø) 22.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 09fcda8...b54db68. Read the comment docs.

divine
divine previously approved these changes Apr 29, 2021
@simonschaufi
Copy link
Contributor Author

@divine Thanx! Is there anything still to do for me or will you merge it?

@divine
Copy link
Contributor

divine commented Apr 29, 2021

@divine Thanx! Is there anything still to do for me or will you merge it?

Hello @simonschaufi ,

We will merge it tomorrow, just waiting for @Smolevich review just in case.

The last push was to fix the link of related PR and nothing is left.

Thanks!

@simonschaufi
Copy link
Contributor Author

simonschaufi commented Apr 29, 2021

Awesome!

@Smolevich Smolevich self-requested a review April 30, 2021 07:19
@Smolevich Smolevich merged commit 114afa0 into mongodb:master Apr 30, 2021
@simonschaufi simonschaufi deleted the patch-1 branch April 30, 2021 07:30
@simonschaufi
Copy link
Contributor Author

@divine @Smolevich can this also be backported into older releases? that would be great!

@divine
Copy link
Contributor

divine commented Apr 30, 2021

@divine @Smolevich can this also be backported into older releases? that would be great!

If you can take care of it, why not 😁.

Thanks!

@divine
Copy link
Contributor

divine commented Apr 30, 2021

Anyways, I might take care of it at the weekend if you're busy @simonschaufi .

Thanks!

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.

passthru is not the sama as laravel (doesntExist method is not working)
5 participants