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

Allow to sync a single model instance #2396

Closed
wants to merge 2 commits into from
Closed

Conversation

mrneatly
Copy link
Contributor

Fix sync() method not handling properly single model's instances passed via $ids argument

Fix `sync()` method not handling properly single model's instances passed via `$ids` argument
@mrneatly
Copy link
Contributor Author

Issue #2395

@codecov-commenter
Copy link

codecov-commenter commented May 28, 2022

Codecov Report

Merging #2396 (c5cdadb) into master (f4c448f) will decrease coverage by 0.04%.
The diff coverage is 50.00%.

@@             Coverage Diff              @@
##             master    #2396      +/-   ##
============================================
- Coverage     87.62%   87.57%   -0.05%     
- Complexity      676      677       +1     
============================================
  Files            31       31              
  Lines          1551     1553       +2     
============================================
+ Hits           1359     1360       +1     
- Misses          192      193       +1     
Impacted Files Coverage Δ
src/Relations/BelongsToMany.php 86.00% <50.00%> (-0.74%) ⬇️

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 f4c448f...c5cdadb. Read the comment docs.

@Smolevich
Copy link
Contributor

@mrneatly do you have plans to continue work with PR?

@mrneatly
Copy link
Contributor Author

mrneatly commented Sep 1, 2022

@Smolevich sure, I just need to figure out what's wrong with the code style. As I know from #2438, there were some issues with StyleCI. Is it somehow related to the result of php-cs-fixer in this PR?

@divine
Copy link
Contributor

divine commented Sep 1, 2022

I can't push changes to your fork as you haven't given any permissions.

You don't have permissions to push to 'mrneatly/laravel-mongodb' on GitHub. Would you like to create a fork and push to it instead?

@mrneatly
Copy link
Contributor Author

mrneatly commented Sep 3, 2022

Hey @divine I've sent you an invitation to my fork

@Smolevich
Copy link
Contributor

Hey @divine I've sent you an invitation to my fork

@mrneatly what is the actual status of this work?

Copy link
Member

@GromNaN GromNaN left a comment

Choose a reason for hiding this comment

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

This seems consistent with what the parent method does in laravel/framework.

Are there any other methods that might need this change?

Could you add a non-regression test to RelationsTest?

@@ -122,7 +123,9 @@ public function sync($ids, $detaching = true)

if ($ids instanceof Collection) {
$ids = $ids->modelKeys();
}
} elseif ($ids instanceof MongodbModel) {
$ids = [$ids->{$this->relatedKey}];
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason for not using InteractsWithPivotTable::parseIds as in the parent class?

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.

5 participants