Skip to content

[8.x] Fix Self-Relation issue in withAggregate method - #35392

Merged
taylorotwell merged 1 commit into
laravel:8.xfrom
khalilst:self-related-withAggregate
Nov 27, 2020
Merged

[8.x] Fix Self-Relation issue in withAggregate method#35392
taylorotwell merged 1 commit into
laravel:8.xfrom
khalilst:self-related-withAggregate

Conversation

@khalilst

@khalilst khalilst commented Nov 27, 2020

Copy link
Copy Markdown
Contributor

This PR is created in following of the PR #35389 .

It modifies the getRelationCountHash to provide a way to predict the hash.
It prevents increasing $selfJoinCount, if called like this:

$relation->getRelationCountHash(true)

@khalilst
khalilst force-pushed the self-related-withAggregate branch 3 times, most recently from 8c62084 to f8bd393 Compare November 27, 2020 17:57
@taylorotwell

Copy link
Copy Markdown
Member

What does $fixed mean? Can that be given an actually descriptive variable name?

@khalilst

khalilst commented Nov 27, 2020

Copy link
Copy Markdown
Contributor Author

@taylorotwell OK. I'm gonna change the $fixed into $lockCount.

@khalilst
khalilst force-pushed the self-related-withAggregate branch from f8bd393 to 07397ce Compare November 27, 2020 19:15
@bjuppa

bjuppa commented Nov 27, 2020

Copy link
Copy Markdown

@khalilst perhaps you can just copy the failing test from #35389 into this PR?

@taylorotwell

Copy link
Copy Markdown
Member

No tests?

@taylorotwell

Copy link
Copy Markdown
Member

Added test.

@taylorotwell
taylorotwell merged commit 07397ce into laravel:8.x Nov 27, 2020
@khalilst

khalilst commented Nov 27, 2020

Copy link
Copy Markdown
Contributor Author

@bjuppa as I have commented in your PR, I was waiting for permission to use your test.
Btw, it was added by @taylorotwell .
Thank you all.

@dbakan

dbakan commented Nov 27, 2020

Copy link
Copy Markdown
Contributor

Could this be a breaking change? Changing the signature of getRelationCountHash? I‘m not sure, just asking.

@khalilst

Copy link
Copy Markdown
Contributor Author

@dbakan No. It's not. Because of the default value of $lockCount = false the previous signature supported.
Also, the instance of the $relation have been checked in withAggregate for special case.

@GrahamCampbell

Copy link
Copy Markdown
Collaborator

The breaking change would be for people extending the class and overriding the method.

@dbakan

dbakan commented Nov 27, 2020

Copy link
Copy Markdown
Contributor

@GrahamCampbell That's what I thought.

Another idea might have been to add a new method getCurrentRelationCountHash.

@dbakan

dbakan commented Nov 27, 2020

Copy link
Copy Markdown
Contributor

@khalilst Just to clarify what I meant: https://3v4l.org/KIleN
But maybe this not a bigger problem. Nice work anyway, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants