CRUD icon indicating copy to clipboard operation
CRUD copied to clipboard

[4.1][Feature] Change Route Key to something besides ID #2212

Open MasterBratac opened this issue 5 years ago • 15 comments

Change crud to use: getKey() -> getRouteKey() getKeyName()->getRouteKeyName()

and update searches to use $this->model->where($this->model->getRouteKey(),$id))

MasterBratac avatar Nov 15 '19 02:11 MasterBratac

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

welcome[bot] avatar Nov 15 '19 02:11 welcome[bot]

I missed edit. I will do more testing and resubmit

MasterBratac avatar Nov 15 '19 13:11 MasterBratac

Wow - this is great work @MasterBratac . Thank you for taking the time to submit this!

tabacitu avatar Nov 17 '19 05:11 tabacitu

@tabacitu Thank you. I was having problems with update and have not had the time to fix that yet.

MasterBratac avatar Nov 17 '19 06:11 MasterBratac

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.

MasterBratac avatar Nov 17 '19 18:11 MasterBratac

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 avatar Nov 17 '19 20:11 MasterBratac

@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 😄

tabacitu avatar Dec 02 '19 11:12 tabacitu

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.

martijnb92 avatar May 07 '20 19:05 martijnb92

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!

tabacitu avatar May 09 '20 03:05 tabacitu

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.

martijnb92 avatar May 09 '20 07:05 martijnb92

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.

srigurubyo avatar Aug 06 '20 12:08 srigurubyo

The progress on this looks great. I apologize I lack the skills to complete this myself.

MasterBratac avatar Aug 20 '20 00:08 MasterBratac

Is this still planned for 4.2? This might be helpful too https://laravel.com/docs/8.x/routing#customizing-the-default-key-name

martijnb92 avatar Jan 19 '21 22:01 martijnb92

@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?

MasterBratac avatar Jan 30 '21 22:01 MasterBratac

@tabacitu I submitted a different pull request for this.

#3490

MasterBratac avatar Jan 30 '21 23:01 MasterBratac

@tabacitu

What about this one? I see that it hasn't been touched since 2021.

EmanueleCoppola avatar Jul 10 '23 09:07 EmanueleCoppola

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.

tabacitu avatar Oct 11 '23 10:10 tabacitu