Laravel-Backpack / PermissionManager

Admin interface for managing users, roles, permissions, using Backpack CRUD
http://backpackforlaravel.com
Other
516 stars 166 forks source link

update UserUpdateCrudRequest #305

Closed otradnix closed 1 year ago

otradnix commented 2 years ago

i guess we have no this config file permission.table_names.users

WHY

BEFORE - What was wrong? What was happening before this PR?

??

AFTER - What is happening after this PR?

??

HOW

How did you achieve that, in technical terms?

??

Is it a breaking change or non-breaking change?

??

How can we test the before & after?

??

welcome[bot] commented 2 years ago

BOOM! Your first PR with us, thank you so much! Someone will take a look at it shortly.

Please keep in mind that:

Thank you!

-- Justin Case The Backpack Robot

pxpm commented 2 years ago

Hello @otradnix

Thanks for the contribution.

We use it with a default value of "users" but probably we can add it on permission file too.

Did you try adding that key to your permission.php config file ? It should work as expected.

If not try running php artisan cache:clear and php artisan config:clear

Let me know, Pedro

otradnix commented 2 years ago

Hello @otradnix

Thanks for the contribution.

We use it with a default value of "users" but probably we can add it on permission file too.

Did you try adding that key to your permission.php config file ? It should work as expected.

If not try running php artisan cache:clear and php artisan config:clear

Let me know, Pedro

Hello, for sure we can add it to config file called 'permission' but isn't better use it like i wrote ? By default permission is empty about table_names.users parameter and you have to review a lot of files before know about it.

Thank's for help with this tiny thing.

pxpm commented 2 years ago

Hey there @otradnix Simple things make the life of a lot of people better. I think you are right in the point that we don't have it in the permission config and that could be misleading to the users, I am just wondering if it would be better to change the code and get the table name from model or expose the config in the file by default so users can change it.

I think the only way to do it in this version is adding it to the config file otherwise it would be a breaking change that we can only push in the next major version.

I will think better about it and tomorrow I will get back here. Thanks for taking the time to submit the PR. Cheers

pxpm commented 1 year ago

Thanks for the PR @otradnix .

I get the issue now 🙏 I think @gleb-svitelskiy PR was better to fix this as Laravel allow us to feed the model and it will call the get table internaly.

I will review and probably merge @gleb-svitelskiy PR that will fix this is a similar maner.

Cheers