CRUD icon indicating copy to clipboard operation
CRUD copied to clipboard

Easily change RouteKey + Easily generate UUID on Model

Open MasterBratac opened this issue 4 years ago • 2 comments

This is a set of 2 traits. --- Easier Implementation of Pull #2223

  1. CanGenerateUuid -- which simply generates a UUID for the model in the column identifed by the public property $uuidColumn. Make sure to add the column to migrations.
  2. CanChangeRouteKey - changes the route key used from the Primary key to something else by setting public property $routeKey. To use a 'uuid' column use public $routeKey = 'uuid'

There are changes to 4 core files to accomplish this.

  1. Add the traits to Backpack\CRUD\app\Models\Traits\CrudTrait
  2. Change the way the 'Read' , 'Update', 'Delete' Traits search for the entry.

I ran the included tests and they pass --- I did not write any new ones. I'm not quite there yet. I did however put it in a project of mine and test it. I did not run into any issues.

Most of the change route key is made possible by overriding Laravel's getRouteKeyName() function.

MasterBratac avatar Jan 30 '21 23:01 MasterBratac

The inspection completed: 10 new issues, 5 updated code elements

scrutinizer-notifier avatar Jan 30 '21 23:01 scrutinizer-notifier

I forgot to mention that the trait canChangeRouteKey also overrides the crud model function getKey()

MasterBratac avatar Jan 31 '21 16:01 MasterBratac

Hi @MasterBratac ,

This is a very clean implementation, well done! Much better than the previous PR 👏👏👏

I'm afraid I can't merge it, though 😔 Why not?

PROs:

  • This is an extra feature that you and someone else mentioned they need, yes. And it's cool.

CONs:

  • it should be considered a BREAKING CHANGE, because if people are already using routeKey in their Models for their customer-facing app... their admin panel will behave differently.
  • it's a relatively small change, but to properly provide support for this, we need to change this in all operations; not only CREATE/READ/UPDATE/DELETE, but BulkDelete, BulkClone, Clone etc; and in operations in first-party packages like Revise; so it's a small change, but in a lot of places;
  • there was small demand for this; only you and 3 other people mentioned needing this, over 4 years;

Because of the PROs & CONs above... I'm afraid it doesn't make sense for us to merge this. I think we just have to say "we do have UUID support, but only when they're used as primary keys".

That being said... there should be workarounds for this that don't need us changing anything in Backpack. Have you considered for the models that have this problem... to extend them and change the primary key to UUID, just for them being used in Backpack? For example you could have AdminProduct extends Product and over there do protected $primaryKey = 'uuid';. Then Backpack should use the UUID as the route key.

I'm sorry for delaying this so much, and in the end saying no. But it doesn't seem like it's worth doing for us. I believe when we created Backpack routeKey didn't even exist. Please don't let this discourage you. I'm shutting down my own PRs too, regularly, it's just the way things go. If you (or anybody else) think I'm wrong here and it does make sense to merge, that it's important for us to do so - let me know and we'll taken another look, re-open. I'm human, I make mistakes all the time.

Sorry for the bad news. But closing this PR we don't plan to merge should clear up our schedule to do something else. Cheers!

tabacitu avatar Oct 11 '23 11:10 tabacitu