[WIP-discussion] Allow show operation to use the panel query#5271
Open
[WIP-discussion] Allow show operation to use the panel query#5271
Conversation
[ci skip] [skip ci]
pxpm
added
Priority: SHOULD
Minor Bug
tabacitu
reviewed
Aug 9, 2023
Member
tabacitu
left a comment
tabacitu
left a comment
There was a problem hiding this comment.
I like the approach yes. A bit scared to do it, but yeah, we should probably do something like this. Only one question
| $this->data['title'] = $this->crud->getTitle() ?? trans('backpack::crud.preview').' '.$this->crud->entity_name; | ||
| if ($this->crud->get('show.usePanelQuery')) { | ||
| $this->data['entry'] = $this->crud->query->withTrashed()->findOrFail($id); | ||
| $this->data['entry'] = $this->crud->setLocaleOnModel($this->data['entry']); |
Member
There was a problem hiding this comment.
Uuuu what does THIS button do?!
And why is this needed here? And only here?
Contributor
Author
There was a problem hiding this comment.
Fair enough. Writing a descriptive comment for you 🙏
Member
There was a problem hiding this comment.
Hmm... something here is fishy to me. Why aren't we using getEntryWithLocale() and we are using setLocaleOnModel()? Looks to me like we're using a setter, instead of a getter.
Member
There was a problem hiding this comment.
Would this do the same thing or no?
Suggested change
| $this->data['entry'] = $this->crud->setLocaleOnModel($this->data['entry']); | |
| $this->data['entry'] = $this->crud->getEntryWithLocale($id); |
[ci skip] [skip ci]
Member
|
Back to you @pxpm |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
WHY
BEFORE - What was wrong? What was happening before this PR?
Heads up: This is working, but it's just a POC to be discussed. We may need to add documentation about it if we decide to do it.
Reported in #5204 and we also had some discussions about it.
In my understanding, and the user reporting the issue, all the operations should be performing their queries over the Panel Query. This would be a huge change in behavior and we may miss use cases that we will break doing such a global change on a non-breaking version.
AFTER - What is happening after this PR?
I've thought about this for a while and decided to "experiment" with the feature in the show operation behind a configuration.
I think this is the way to go now, without risking breaking all people apps. We may eventually turn it
trueby default in a future version.HOW
How did you achieve that, in technical terms?
I introduced the
usePanelQueryconfig in show operation, that will tell the operation if it should perform thefindOrFailin the model, or in panel query.Is it a breaking change?
I don't think so no, it need to be enabled.
How can we test the before & after?
Steps described in the linked issue. But to sum up, create a constrain on the crud query and watch it being applied in the List but not in the show operation.