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