-
Notifications
You must be signed in to change notification settings - Fork 171
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
Apply conditions/scopes to related model query #536
Conversation
@Flynsarmy can you confirm this fixes your problem? |
This PR helps but there still appears to be a bug which you can replicate with my test plugin in octobercms/october#4952. Uncheck both Tag checkboxes but leave one or more Category checkboxes checked. Hit save and refresh. You'll notice the categories did not save correctly. A look in the DB confirms no categories were saved to the |
@Flynsarmy try this, I think I fixed the remaining bug. |
@mjauvin Looks good! Fixed. |
I and @Flynsarmy have tested this as a working fix. |
A unit test for the original issue would be helpful here @mjauvin |
@LukeTowers there are tests in october/october for testing relations, would that make sense to improve on those tests instead of rewriting these in october/library? |
Might be, any thoughts @bennothommo? |
@mjauvin @LukeTowers the test should really exist in here if the changed functionality is here - while it's likely that 99.9% of the time, this will be used in October CMS, it's still proper testing procedure for this sort of functionality to be tested in isolation. I would simply port a couple of those relation tests back into this library and adjust as necessary. |
@bennothommo I think we might need your skills to setup database tests for this library then, I was having a heck of time trying to make it work for the guarded tests |
@LukeTowers I found this:
That should help |
@LukeTowers what @mjauvin posted is the only way of testing database functions that I'm aware of with the library. Give that a try and let me know if it doesn't work - I'll have to port some of the functionality from October CMS back into the library. I swear I did this originally with the L6 development, but had it reversed afterwards - c'est la vie :(. |
Should be good now! |
@mjauvin Just to eliminate any doubt, would you mind adding a couple more scenarios in the tests, namely:
If those are covered, I'm happy to merge. |
Also, I'm going to merge your DbTest changes manually into |
Refs: #536 Co-authored-by: Marc Jauvin <marc.jauvin@gmail.com>
Co-authored-by: Ben Thomson <git@alfreido.com>
…fix-sync-relations
The tests have been added @bennothommo |
Anything else I need to do to here? |
Fixes: octobercms/october#4952
This overrides the following method in Laravel:
https://github.com/laravel/framework/blob/8.x/src/Illuminate/Database/Eloquent/Relations/Concerns/InteractsWithPivotTable.php#L83
Specifically replaces the following lines to fetch the records that actually belongs to the relation (using the conditions/scopes of the relation):
https://github.com/laravel/framework/blob/8.x/src/Illuminate/Database/Eloquent/Relations/Concerns/InteractsWithPivotTable.php#L92-L93