Support strings as numeric ids

[erikn69]

testing #2530 (comment)

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

laravel-permission/tests/WildcardHasPermissionsTest.php

Line 244 in 62f22e1

$userPermission = Permission::create(['name' => '8']);

---

On v5 was is_int, an is_string, so string as "42" always is taken as name

laravel-permission/src/Traits/HasPermissions.php

Line 138 in d829888

$method = is_string($permission) ? 'findByName' : 'findById';

laravel-permission/src/Traits/HasPermissions.php

Lines 156 to 168 in d829888

	if (is_string($permission)) {
	$permission = $permissionClass->findByName(
	$permission,
	$guardName ?? $this->getDefaultGuardName()
	);
	}
	if (is_int($permission)) {
	$permission = $permissionClass->findById(
	$permission,
	$guardName ?? $this->getDefaultGuardName()
	);
	}

[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:
   

   laravel-permission/src/Traits/HasPermissions.php

   Lines 491 to 503 in 62f22e1

   	if (is_array($permissions)) {
   	$permissions = array_map(function ($permission) {
   	if ($permission instanceof \BackedEnum) {
   	return $permission->value;
   	}
   	return is_a($permission, Permission::class) ? $permission->name : $permission;
   	}, $permissions);
   	return $this->getPermissionClass()::whereIn('name', $permissions)
   	->whereIn('guard_name', $this->getGuardNames())
   	->get();
   	}

   
   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?

laravel-permission/src/Traits/HasPermissions.php

Lines 436 to 439 in 62f22e1

	public function syncPermissions(...$permissions)
	{
	if ($this->getModel()->exists) {
	$this->collectPermissions($permissions);

laravel-permission/src/Traits/HasPermissions.php

Lines 361 to 368 in 62f22e1

	return collect($permissions)
	->flatten()
	->reduce(function ($array, $permission) {
	if (empty($permission)) {
	return $array;
	}
	$permission = $this->getStoredPermission($permission);

On revokePermissionTo it will fail

laravel-permission/src/Traits/HasPermissions.php

Lines 453 to 455 in 62f22e1

	public function revokePermissionTo($permission)
	{
	$this->permissions()->detach($this->getStoredPermission($permission));

[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.

laravel-permission/tests/WildcardHasPermissionsTest.php

Line 244 in 62f22e1

$userPermission = Permission::create(['name' => '8']);

[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)

laravel-permission/src/Traits/HasPermissions.php

Lines 444 to 450 in d829888

	if (is_numeric($permissions)) {
	return $permissionClass->findById($permissions, $this->getDefaultGuardName());
	}
	if (is_string($permissions)) {
	return $permissionClass->findByName($permissions, $this->getDefaultGuardName());
	}

laravel-permission/src/Traits/HasRoles.php

Lines 319 to 325 in d829888

	if (is_numeric($role)) {
	return $roleClass->findById($role, $this->getDefaultGuardName());
	}
	if (is_string($role)) {
	return $roleClass->findByName($role, $this->getDefaultGuardName());
	}

[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

[spatie-bot]

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.

[hichemfantar]

Is this still relevant?

[drbyte]

Is this still relevant?

Yes.

[spatie-bot]

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.