CRUD icon indicating copy to clipboard operation
CRUD copied to clipboard

Optimize crud READ operations (list/show)

Open pxpm opened this issue 3 years ago • 4 comments

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.description should 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 parseTranslations method.
  • eager loading the relationship columns lead to the removal of getRelatedEntries and 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.

pxpm avatar Jul 29 '22 11:07 pxpm

@tabacitu what do you think about this ?

pxpm avatar Jul 29 '22 11:07 pxpm

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_multiple in favor of select (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 🙏

tabacitu avatar Aug 03 '22 07:08 tabacitu

I'm going to close this, but not delete the branch. So that @pxpm you can re-use this code.

tabacitu avatar Aug 03 '22 07:08 tabacitu

Actually let's keep this open until you get started on the PRs.

tabacitu avatar Aug 03 '22 07:08 tabacitu