Search code, repositories, users, issues, pull requests...

Copy link

Contributor

@hans-thomas hans-thomas commented Nov 7, 2023 •

edited

Loading

Hi,
I continued #2640 PR, thanks to @ithuis. I fixed the relationship and its inverse way,
but it still needs more tests for

attaching a collection of models
detaching action
finally syncing action
There is a problem with loading relation using load method

It has to be done by tomorrow.

ithuis and others added 30 commits

October 12, 2023 17:09

@ithuis @medspec


 add MorphToMany

8eaad01

attach MorphToMany working
detaching works
sync working
cleanup

@ithuis @medspec


 requested code cleanup

acfdb19

@ithuis


 Merge branch '4.1' into morphToMany

5a55f80

WIP

d929456

WIP

dd8e701


 Add tests for MorphToMany and MorphedByMany;

17a3aca


 Improve DX;

4e7ad77


 Merge branch '4.1' into 2640-morph-to-many

07afe18

# Conflicts:
#	tests/RelationsTest.php


 fix cs;

f8240e2


 Merge branch '4.1' into 2640-morph-to-many

b9ae8ab


 Querying logic in inverse mode updated;

fd0a9f9


 Attaching a collection or string id(s) fixed;

dbe93d0


 Add tests for attaching a collection;

dc22fc9


 Add tests for attaching multiple string ids;

a0cb480

WIP

9f888ab


 Fix CS;

3d4abab


 Detaching fixed;

24119d7


 Add tests for detaching;

1833c5a


 Add tests for detaching multiple ids;

ad1c6ae


 Update MorphToMany.php

26f7ad2


 Add test for syncing a mode;


 Add test for syncing a collection of models;

dbaab5f


 Add test for syncing multiple ids;

a2c6407

WIP

284b8c3


 Fix syncing in inverse mode

92040cc


 Add test for syncing a model;

79e5618


 Add test for syncing a collection of models;

5ea4aef


 Add test for syncing multiple id;


 Fix CS;

f655f72


 Fix eager loading relation and refreshing the model before lazy loading;

4932b3b

hans-thomas

hans-thomas commented

Nov 16, 2023

src/Relations/MorphToMany.php

Comment on lines +36 to +40

/** @inheritdoc */

public function getRelationExistenceQuery(Builder $query, Builder $parentQuery, $columns = ['*'])

{

return $query;

}

Copy link

Contributor Author

@hans-thomas hans-thomas Nov 16, 2023

Choose a reason for hiding this comment

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

I think the parent method is fine and we don't need to override this. I will write test for this too.

Copy link

Contributor Author

@hans-thomas hans-thomas Nov 18, 2023

Choose a reason for hiding this comment

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

This one is related to HybridRelations and never called in my tests.

hans-thomas added 13 commits

November 18, 2023 11:32


 extractIds method returns unique ids;

5d229eb


 getHasCompareKey updated;

b56b90e


 Handling MorphToMany has query;

ca1cc62


 Add tests for handling has query in MorphToMany relationship;

5d77236


 Handling MorphedByMany has query;

9618b84


 getHasCompareKey removed;

6d6a99e

We don't need this method anymore;


 Add test for MorphedByMany has query;

9f53b26


 Fix handling custom MorphedByMany has query;

c4ca146


 Add test for MorphByMany has query;

989b263


 Revert "extractIds method returns unique ids;"

1817c94

This reverts commit 5d229eb.


 Make extractIds method public;

fc53e7d


 Add test for handling custom MorphToMany has query;

d9dedf6


 Fix CS;

800bfac

hans-thomas

hans-thomas commented

Nov 18, 2023

src/Helpers/QueriesRelationships.php

Comment on lines -117 to +125

$relations = $hasQuery->pluck($this->getHasCompareKey($relation));

$relations = match (true) {

$relation instanceof MorphToMany => $relation->getInverse() ?

$this->handleMorphedByMany($hasQuery, $relation) :

$this->handleMorphToMany($hasQuery, $relation),

default => $hasQuery->pluck($this->getHasCompareKey($relation))

};

Copy link

Contributor Author

@hans-thomas hans-thomas Nov 18, 2023

Choose a reason for hiding this comment

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

I added a match expression to handle the MorphToMany relations has query.

alcaeus

alcaeus approved these changes

Nov 20, 2023

Copy link

Member

@alcaeus alcaeus left a comment

Choose a reason for hiding this comment

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

Just a minor comment. Thanks for working on this @hans-thomas, and I really appreciate the amount of comments you added to the code! ❤️

src/Eloquent/HybridRelations.php Outdated Show resolved Hide resolved

Copy link

Contributor Author

hans-thomas commented Nov 20, 2023

@alcaeus My pleasure❤️

@hans-thomas @alcaeus


 Parent morphToMany call with a minuscule "m";

51b9e2a

Co-authored-by: Andreas Braun <alcaeus@users.noreply.github.com>

GromNaN

GromNaN reviewed

Nov 21, 2023

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.

LGTM. A last question about the commented code and minor CS fixes.

src/Helpers/QueriesRelationships.php Show resolved Hide resolved

tests/RelationsTest.php Outdated Show resolved Hide resolved

src/Relations/MorphToMany.php

if ($this->getInverse()) {

// Attach the new ids to the parent model.

$this->parent->push($this->table, [

Copy link

Member

@GromNaN GromNaN Nov 20, 2023

Choose a reason for hiding this comment

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

Great 👍🏻 It seems that you have tested this case carefully.

hans-thomas and others added 3 commits

November 22, 2023 14:01

@hans-thomas @GromNaN


 Add an empty line before return in handleMorphToMany method;

cc5bbcf

Co-authored-by: Jérôme Tamarelle <jerome@tamarelle.net>

@hans-thomas @GromNaN


 Add an empty line before return in handleMorphedByMany method;

92e6389

Co-authored-by: Jérôme Tamarelle <jerome@tamarelle.net>


 Uncomment the checks of testHasManyHas;

9198df9

@GromNaN GromNaN mentioned this pull request

added MorphToMany with testcases #2640

Closed

@GromNaN


 Merge branch '4.1' into 2640-morph-to-many

44c2517

GromNaN

GromNaN approved these changes

@GromNaN GromNaN merged commit 82ddb83 into mongodb:4.1

@GromNaN

Copy link

Member

GromNaN commented Nov 22, 2023

Thank you so much @hans-thomas

@hans-thomas hans-thomas deleted the 2640-morph-to-many branch

November 22, 2023 11:38

Copy link

Contributor Author

hans-thomas commented Nov 22, 2023

@GromNaN I'm glad I could help❤️

Treggats pushed a commit to Treggats/laravel-mongodb that referenced this pull request