CRUD
CRUD copied to clipboard
[4.1][Feature] Change Route Key to something besides ID #2212
Change crud to use: getKey() -> getRouteKey() getKeyName()->getRouteKeyName()
and update searches to use $this->model->where($this->model->getRouteKey(),$id))
BOOM! Your first PR with us, thank you so much! Someone will take a look at it shortly.
Please keep in mind that:
- if this constitutes a breaking change, it might take quite a while for this to get merged; we try to emulate the Laravel release cycle as much as possible, so developers can upgrade both software once; this means a new big release every ~6 months;
- even if it's a non-breaking change, it might take a few days/weeks for the PR to get merged; unless it's a no-brainer, we like to have some community feedback on new features, before we merge them; this leads to higher-quality code, in the end; we learnt this the hard way :-)
- not all PRs get merged; sometimes we just have to hold out new features, to keep the packages lean; sometimes we don't include features that only apply to niche use cases;
- we're not perfect; if you think we're wrong, call us out on it; but in a kind way :-) we all make mistakes, best we learn from them and build better software together;
Thank you!
-- Justin Case The Backpack Robot
I missed edit. I will do more testing and resubmit
Wow - this is great work @MasterBratac . Thank you for taking the time to submit this!
@tabacitu Thank you. I was having problems with update and have not had the time to fix that yet.
The update function needs to stay as $item = $this->model->findOrFail($id);//This will always use primary key. But since it is POST ok.
this is what is passed by the request.
I have a pull request into Laravel for the following which could also be easily implemented in the base crud class to change the route key by
protected $routeKeyName = 'uuid';
in the crud model. simply by changing the getRouteKeyName() function
public function getRouteKeyName()
{
return (isset($this->routeKeyName) ? $this->routeKeyName : $this->getKeyName());
}
Let's see if they merge that or not. If not I may open a pull request for that later.
if @tabacitu might be interested in that.
@MasterBratac that sounds very interesting indeed - can you please link to the Laravel PR? Did it get merged? If so, I believe the changes needed in Backpack will be much smaller, which would be great 😄
Looking forward to this Backpack feature. I'm implementing UUID for some models but all relations are based on ID, so I will need to continue to use both.
Oooh... 🤯 now I understand!
Thanks for pitching in @martijnb92 ... I never prioritised this because I didn't see the particular use case where this is crucial. I think I understand now, please correct me if I'm wrong:
- your models have UUIDs, so you want to show those in the route, not IDs;
- your models also have IDs, set as primary keys, because your relationships are based on them;
The only way to solve this right now, would be to
- make sure you define all parameters of your relationships - including the keys
- set the UUIDs as the primary keys Then the relationships won't be affected, they'll use the provided IDs. Right?
But what really needs to happen is that we should fix this, and use getRouteKey()
and getRouteKeyName()
- I agree. Thanks!
That's exactly the case. It does work if edit all the relations with the parameters and set UUID as primary etc.., but I prefer to use getRouteKeyName()
instead. Looks much cleaner.
We do have a similar usecase where all relationships are based on 'id' however want to expose UUID based routes.
This is an interesting feature and we are looking forward to get this implemented in backpack.
The progress on this looks great. I apologize I lack the skills to complete this myself.
Is this still planned for 4.2? This might be helpful too https://laravel.com/docs/8.x/routing#customizing-the-default-key-name
@tabacitu There is now a much more simple implementation for this on the current branch and the ability to override
getRouteKeyName()
in laravel. 3 small changes to Backpack, override the getKey()
function from the model, and add 2 a trait. Would you like it as a new pull request?
@tabacitu I submitted a different pull request for this.
#3490
@tabacitu
What about this one? I see that it hasn't been touched since 2021.
Thanks for re-surfacing this @EmanueleCoppola . I see @MasterBratac has submitted https://github.com/Laravel-Backpack/CRUD/pull/3490 that is a newer PR than this. I'll close this PR and take a look at that one right now. Let's move the conversation there.