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

[10.x] Allow utilising withTrashed(), withoutTrashed() and onlyTrashed() on MorphTo relationship even without SoftDeletes Model #47880

Merged
merged 10 commits into from
Aug 15, 2023

Conversation

crynobone
Copy link
Member

@crynobone crynobone commented Jul 28, 2023

This would be useful in some 3rd party packages as this has become an issue where it's not possible to define the relationship such as $this->morphTo()->withTrashed() unless all relationship uses SoftDeletes trait:

Test without and with code changes

image

Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com>
Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com>
Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com>
Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com>
Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com>
Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com>
@crynobone crynobone marked this pull request as draft July 28, 2023 13:52
Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com>
Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com>
@crynobone crynobone marked this pull request as ready for review July 28, 2023 14:06
@taylorotwell
Copy link
Member

@crynobone you mean all of the morphable models have to be soft deletable?

@crynobone
Copy link
Member Author

Before this changes if we want to use morphTo()->withTrashed() and then query using Action::with('target') all morphable model need to use soft deletes or it would throw an exception (similar to failed test).

This work well for application models but if a package provides morphable relationship and doesn't fully know which model would use it they would faced this issue.

@matiaslauriti
Copy link
Contributor

matiaslauriti commented Jul 31, 2023

Why would a model do nothing when withTrashed etc is called if it does not have the SoftDeletes? Is like saying "I want to eat vegetables and rice, but I only have a rice, I still expect to remove the lid and see vegetables in there"... the package should check or do something related to those non-existing methods, else withTrashed should return the same query builder but without changing anything, and I would say that is not a good behaviour because then, you would need to know that it does not implement SoftDeletes so it would do nothing at all...

I may still be confused or something similar, but still... it is not good at all to call a method and do nothing... it should throw an error if it is not implemented but not implement an "empty" method, right?

@crynobone
Copy link
Member Author

Why would a model do nothing when withTrashed etc is called if it does not have the SoftDeletes?

This has already been done on Route::withTrashed() and the same argument can be applied.

@taylorotwell taylorotwell merged commit 294661d into laravel:10.x Aug 15, 2023
19 checks passed
@crynobone crynobone deleted the patch-morphto-softdeletes branch August 18, 2023 01:31
@mreduar
Copy link

mreduar commented Aug 23, 2023

This is great, thank you very much, I have been dealing with this problem for a while now.

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