[10.x] Fix infinite loop when global scopes query contains aggregates

[mateusjunges]

Attempt to fix #49968.

TLDR: We can't call $this->getGrammar() directly because this causes an infinite loop.

Pinging @tpetry here because this is related to #49912. I'm not sure how to test this TBH, so I'd appreciate your input here.

Calling $this->getGrammar() will redirect the call (passthru) from \Illuminate\Database\Eloquent\Builder to the base Illuminate\Database\Query\Builder. When doing this, we add a call to ->applyScopes():

framework/src/Illuminate/Database/Eloquent/Builder.php

Line 1760 in 4f7802d

return $this->applyScopes()->getQuery();

This will cause laravel to call the scopes we have defined, and, since we are using a count aggregate, we'll end up in the withAggregate method defined on line 606 of Illuminate\Database\Eloquent\Concerns\QueriesRelationships trait. Now, since we are calling $this->getGrammar() directly, we'll start the process of forwarding the call to the base query builder and calling ->applyScopes all over again, which will cause an infinite loop.

I hope I was clear enough, but help to explain in the comments if need be.

[mateusjunges]

I created a repo to reproduce the issue: https://github.com/mateusjunges/reproduce-laravel-49968. Just install the repo locally and run the tests.

[bilogic]

@mateusjunges This is a life saver!

[bilogic]

@mateusjunges This is a life saver!

[tpetry]

Sorry @mateusjunges. I am always embarrassed when breaking something. I didn‘t think on this edge case when writing that patch. There are so many ways you can break the database part and many code paths don‘t have a unit test. Can you PR a unit test for this case so we have some guarantee we can‘t break this behaviour in the future again with some other changes.

[mateusjunges]

@tpetry no worries man, I mentioned just to make sure this was the correct fix, you have way more experience than me with the database layer 😁

I'll work on a PR for adding unit tests for this

[mxts]

This will cause laravel to call the scopes we have defined, and, since we are using a count aggregate, we'll end up in the withAggregate method defined on line 606 of Illuminate\Database\Eloquent\Concerns\QueriesRelationships trait. Now, since we are calling $this->getGrammar() directly, we'll start the process of forwarding the call to the base query builder and calling ->applyScopes all over again, which will cause an infinite loop.

wow thanks @mateusjunges

Did you use any particular method to track this down? I'm just so amazed at how quickly you fixed it when it took me almost 3 days just to replicate it.

[mateusjunges]

@mxts it's basically tracking it down to which PR broke the expected behavior. I usually do it installing the project locally and the you can use Git to go back in time to previous versions of the code. Once you know which PR caused the issue it gets easier as you have the diff. This gives me a general idea about what could be the issue. Then I use xdebug to track it down to the root cause.