Skip to content

permission revoke based on ids as well#2552

Closed
tariqali021 wants to merge 4 commits intospatie:mainfrom
tariqali021:revoke-permission-enhancement
Closed

permission revoke based on ids as well#2552
tariqali021 wants to merge 4 commits intospatie:mainfrom
tariqali021:revoke-permission-enhancement

Conversation

@tariqali021
Copy link
Copy Markdown

No description provided.

@parallels999
Copy link
Copy Markdown
Contributor

There is no change

@tariqali021
Copy link
Copy Markdown
Author

There is no change

Added missing logic.. Need to support both using id & name when revoking permission. Ideally this can be a mixture of id & name that could also be done. Thanks to pointing out the missing code :-)

@parallels999
Copy link
Copy Markdown
Contributor

parallels999 commented Nov 13, 2023

Still, is 09ebf39 a refactor?
Needs some tests

@tariqali021
Copy link
Copy Markdown
Author

Yes I found some tweaks & fix it.. my docker is crashing & unable to test.. would be great if you review it carefully..

@parallels999
Copy link
Copy Markdown
Contributor

Seems that #2548 does the same, test it

@tariqali021
Copy link
Copy Markdown
Author

That's something else.. This change belongs to revoking permission given as int (primary key of permission table). Currently, it is only supporting based on name.

@parallels999
Copy link
Copy Markdown
Contributor

This change belongs to revoking permission given as int (primary key of permission table). Currently, it is only supporting based on name.

That is exactly what #2548 is adding

@tariqali021
Copy link
Copy Markdown
Author

Yes it is also doing the same what I'm trying to achieve but to me it seems not optimized way to do that.. correct me if I'm wrong. That code is finding each individual record from db either based on it or string. but the good thing is that it is revoking based on both primary key & name column

@parallels999
Copy link
Copy Markdown
Contributor

That code is finding each individual record from db either based on int or string

On removeRole yes, on revokePermissionTo no, permissions are getting from cache, not from DB

@tariqali021
Copy link
Copy Markdown
Author

yes but for very rare and edge case if we don't have cache stored at certain point. but still acceptable because of edge case.

@parallels999
Copy link
Copy Markdown
Contributor

parallels999 commented Nov 13, 2023

The documentation should also be updated, it says that it only accepts string or Permission object

The `givePermissionTo` and `revokePermissionTo` functions can accept a
string or a `Spatie\Permission\Models\Permission` object.

@tariqali021
Copy link
Copy Markdown
Author

yes and also enum

@spatie-bot
Copy link
Copy Markdown

Dear contributor,

because this pull request seems to be inactive for quite some time now, I've automatically closed it. If you feel this pull request deserves some attention from my human colleagues feel free to reopen it.

@spatie-bot spatie-bot closed this Mar 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants