Optimize crud READ operations (list/show)
WHY
BEFORE - What was wrong? What was happening before this PR?
[1] - Show didn't eager load relationships List does, why not show too ?
[2] - Show/List couldn't display entries in other languages Example:
- Article model translatable.
- I am in Article show.
- Article HasOne Publisher.
- Publisher have description translatable.
My column in ArticleCrudController:
[
'name' => 'publisher.description'
]
What I expect?
- If I am viewing Article in French,
publisher.descriptionshould be in french too if translation exists, otherwise fallback to the one that exists.
AFTER - What is happening after this PR?
Basically the biggest change in terms of code size was eager loading the relationships in show too, that lead to removing ALOT of code that would otherwise be needed if relationships were not eager loaded.
Another important change is that anytime we deal with models we make sure to add the locale and/or if useFallbackLocale should be used.
This is because this PR is a continuation of: https://github.com/Laravel-Backpack/CRUD/pull/4267
After this two PR's we will not be using $crud->model-> anywhere, we always use the $crud->query that user can customize on each of their operations.
select_multiple column is deprecated, select handles all scenarios.
HOW
How did you achieve that, in technical terms?
Refactoring code.
- the fact that we now add the locale on model lead us to remove the
parseTranslationsmethod. - eager loading the relationship columns lead to the removal of
getRelatedEntriesand the getAttributes refactor.
Is it a breaking change?
Well .. I guess we should consider it a breaking change, but I think it's atmost a behavioral change than a breaking one.
@tabacitu what do you think about this ?
I agree to doing this, and I agree to doing this now in v5.2. But as it is, this PR is too difficult to review and merge.
I see this removes the public method getModelWithCrudPanelQuery() so right off the bat, it's a BC. But. I think a lot of this (if not everything) can be merged as non-BC, if we redo this as multiple, non-BC PRs. For example:
- one PR to deprecate
select_multiplein favor ofselect(that's an easy merge); - one PR to change the Show operation from
getModel()to$this->crud->query(which is arguable, don't I remember we did this and then reverted?) - one PR to add the eager load
- one PR to add the fallback locale (maybe)
Again, I agree we should do all of this. But not like this - in individual PRs. Remember the golden rule Pedro - Each PR does only one thing. I could tell this PR is un-mergeable from
- the name
- "optimize" is a red flag, what you wanted to say there was "refactor" but tried to hide it 😅 cheeky cheeky!
- this does something from both the LIST and SHOW operations (which granted if it was a small thing would have been ok);
- the body lists all the things that are changed, which are... five things 😅;
That makes this too difficult to test and review, sorry. Again, I like this. It looks ok at first glance. But I won't be able to sleep for 1 month if I merge something with this many changes in crucial parts of the core. So let's redo this as atomic PRs please 🙏
I'm going to close this, but not delete the branch. So that @pxpm you can re-use this code.
Actually let's keep this open until you get started on the PRs.