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 passing query Expression as column in Many-to-Many relationship #50849

Merged
merged 1 commit into from
Apr 1, 2024

Conversation

plumthedev
Copy link
Contributor

@plumthedev plumthedev commented Mar 29, 2024

Query builder supports query expressions when Many-to-Many relationships.
I have fixed that by check if passed column to be qualified with explicit table name is query expression, if so I made a return to use it as developer requested, without qualifying. Added support for expression in clauses methods types. Covered by test which is verifying if base relation methods receives expression.
Moreover, PHPUnit test case replaced with Mockery adapter test case, which closes Mockery by themself.

This MR solves #50787

@tpetry
Copy link
Contributor

tpetry commented Mar 30, 2024

Please use the expression functions from the query grammar. These should be isExpression() and getValue().

And please create tests so this issue can‘t be re-introduced anymore.

@plumthedev plumthedev force-pushed the fix/50787 branch 2 times, most recently from e52430b to 150418a Compare March 30, 2024 13:09
@plumthedev
Copy link
Contributor Author

Dear @tpetry
Thanks for advice, added. Please check it now.

@tpetry
Copy link
Contributor

tpetry commented Mar 31, 2024

Looks better 👍

@crynobone crynobone linked an issue Mar 31, 2024 that may be closed by this pull request
@GrahamCampbell
Copy link
Member

I'm not so sure this change belongs in the framework core. The use case seems pretty edge case.

@GrahamCampbell
Copy link
Member

This is also a breaking change for existing packages that extend the framework's relationship classes.

@plumthedev
Copy link
Contributor Author

I believe that is not a BC as I have fixed edge case which was not working properly. Even if third parties used expressions they are not working correctly

@tpetry
Copy link
Contributor

tpetry commented Mar 31, 2024

For 10.x we fixed several cases were expressions had not been handled correctly. To be more clear, 10.x added the bc break because my expression could only handle correctly everything i new off. But expressions could have been used at a ton of places - normally not documented. So in my opinion, this is fixing a bug like many times before.

@taylorotwell taylorotwell merged commit 910d64d into laravel:11.x Apr 1, 2024
28 checks passed
@ccharz
Copy link
Contributor

ccharz commented Apr 4, 2024

This change seems to cause an issue when using an belongsToMany relation in combination with "withoutGlobalScopes". In my case the global scopes are not removed. If i revert this change, all works as expected.

Would it be possible to change the call from $this->getGrammar()->isExpression($column) to $this->query->getQuery()->getGrammar()->isExpression($column) so the getGrammar() call doesn't get forwarded to the builders toBase() method (which applies all the scopes)?

@tpetry
Copy link
Contributor

tpetry commented Apr 5, 2024

@ccharz Please create a new issue with a reproducible example. This bug get easily missed as a simple comment.

@ccharz
Copy link
Contributor

ccharz commented Apr 6, 2024

@tpetry Okay - I have created the issue #50945

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.

Using expressions in pivot queries
5 participants