Support strings as numeric ids

[erikn69]

testing https://github.com/spatie/laravel-permission/issues/2530#issuecomment-1779319899

Closes #2530 Closes #2543 Closes #2541

pro: this is consistent with laravel sync method, but on this part it suggests that you can use int string as name (on wildcards), even so, wildcards seems to work, I don't know if it needs more tests https://github.com/spatie/laravel-permission/blob/62f22e192711fe56fb55b57b586001f8674b4396/tests/WildcardHasPermissionsTest.php#L244

---

On v5 was is_int, an is_string, so string as "42" always is taken as name https://github.com/spatie/laravel-permission/blob/d829888b659cc7df9a7d10ad29f24cbfeb6c3297/src/Traits/HasPermissions.php#L138 https://github.com/spatie/laravel-permission/blob/d829888b659cc7df9a7d10ad29f24cbfeb6c3297/src/Traits/HasPermissions.php#L156-L168

[drbyte]

I think we need to add some tests for this situation we're trying to fix.

A couple observations:

1. The reported situations are where the data being passed is from form input: ids sent as strings via POST.
2. The getStoredPermissions() method doesn't check against IDs if the incoming param is an array; it only checks against names: https://github.com/spatie/laravel-permission/blob/62f22e192711fe56fb55b57b586001f8674b4396/src/Traits/HasPermissions.php#L491-L503 It also checks against ALL guards ... hmmm....

[erikn69]

the getStoredPermissions() method doesn't check against IDs if the incoming param is an array

Ok, i see it now, but syncPermissions calls collectPermissions, then collectPermissions calls getStoredPermissions collectPermissions uses flatten before getStoredPermission, In that case, would it be a string instead of an array?

https://github.com/spatie/laravel-permission/blob/62f22e192711fe56fb55b57b586001f8674b4396/src/Traits/HasPermissions.php#L436-L439 https://github.com/spatie/laravel-permission/blob/62f22e192711fe56fb55b57b586001f8674b4396/src/Traits/HasPermissions.php#L361-L368 On revokePermissionTo it will fail https://github.com/spatie/laravel-permission/blob/62f22e192711fe56fb55b57b586001f8674b4396/src/Traits/HasPermissions.php#L453-L455

[erikn69]

I added some tests but I'm still not sure, I feel like I'm missing something 😕

[parallels999]

Could this be a breaking change?

[drbyte]

Could this be a breaking change?

As in need to bump to 7.0 immediately?

[erikn69]

Could this be a breaking change?

Possibly, if someone uses numbers as names in permissions/roles, they will no longer be able to access those models through the package.

https://github.com/spatie/laravel-permission/blob/62f22e192711fe56fb55b57b586001f8674b4396/tests/WildcardHasPermissionsTest.php#L244

[drbyte]

Possibly [a breaking change], if someone uses numbers as names in permissions/roles, they will no longer be able to access those models through the package.

I wonder if that scenario could be simply handled with a configuration switch? IMO the number-as-a-name is a rare-use case, but number-as-model-id-integer is vast majority use-case.

[parallels999]

IMO the number-as-a-name is a rare-use case, but number-as-model-id-integer is vast majority use-case.

Yes, most likely

[parallels999]

After doing some digging it is not a breaking change(v5), it is a bug fix, #2404 break this(lack of tests) https://github.com/spatie/laravel-permission/blob/d829888b659cc7df9a7d10ad29f24cbfeb6c3297/src/Traits/HasPermissions.php#L444-L450 https://github.com/spatie/laravel-permission/blob/d829888b659cc7df9a7d10ad29f24cbfeb6c3297/src/Traits/HasRoles.php#L319-L325

[massiws]

Same issue here upgrading from v.5.5 to v.6.2.

Simple test in tinker:

<?php

$permissions = ["10", "20", "30"];
$user->syncPermissions($permissions);

// v.5.5 => no error
// v.6.2 => error: Spatie\Permission\Exceptions\PermissionDoesNotExist  There is no [permission] named `10` for guard `web`.

Adopting the changes proposed in this PR v.6.2 works fine.

[hichemfantar]

Same issue here upgrading from v.5.5 to v.6.2.

Simple test in tinker:

<?php

$permissions = ["10", "20", "30"];
$user->syncPermissions($permissions);

// v.5.5 => no error
// v.6.2 => error: Spatie\Permission\Exceptions\PermissionDoesNotExist  There is no [permission] named `10` for guard `web`.

Adopting the changes proposed in this PR v.6.2 works fine.

Looks like this fixes a regression in v6, any plans for this to be merged soon? Is there anything missing in this PR?

[axlon]

@erikn69 @drbyte any updates on if/when this will be merged?

[drbyte]

Status: Trying to decide whether this is safe to merge into v6, or whether to tag it as v7 .... and then what the complaints will be about v7 :(

[hichemfantar]

Status: Trying to decide whether this is safe to merge into v6, or whether to tag it as v7 .... and then what the complaints will be about v7 :(

Should probably be v7 to avoid issues with existing installs