Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better handle sitatuations where a user's role on a team is undefined #905

Merged
merged 2 commits into from
Oct 28, 2021
Merged

Better handle sitatuations where a user's role on a team is undefined #905

merged 2 commits into from
Oct 28, 2021

Conversation

stevegrunwell
Copy link
Contributor

@stevegrunwell stevegrunwell commented Oct 28, 2021

In some situations (especially when testing using database factories), the "role" column on the team_user table may be left NULL. As a result, attempting to check the user's permissions based on their role causes an error, as Jetstream::findRole() requires a string.

For example:

use App\Models\Team;
use App\Models\User;

$team = Team::factory()
	->with(User::factory())
	->create();

$team->users->first()->hasTeamPermission($team, 'read');
#=> TypeError: Laravel\Jetstream\Jetstream::findRole(): Argument #1 ($key) must be of type string, null given, called in /vendor/laravel/jetstream/src/HasTeams.php on line 145

This PR ensures that Jetstream::findRole() is only called if we have a corresponding role, preventing this error. Similarly, teamPermissions() will no longer cause a "ErrorException: Attempt to read property "permissions" on null" exception when the role is undefined.

In some situations (especially when testing using database factories), the "role" column on the team_user table may be left NULL. As a result, attempting to check the user's permissions based on their role causes an error, as `Jetstream::findRole()` requires a string.

For example:

```php
use App\Models\Team;
use App\Models\User;

$team = Team::factory()
	->with(User::factory())
	->create();

$team->users->first()->hasTeamPermission($team, 'read');
```

This commit ensures that `Jetstream::findRole()` is only called if we have a corresponding role, preventing this error. Similarly, `teamPermissions()` will no longer cause a "ErrorException: Attempt to read property "permissions" on null" exception when the role is undefined.
@driesvints
Copy link
Member

Seems to breaks the test suite. Please mark this PR as ready for review when they're passing again.

@driesvints driesvints marked this pull request as draft October 28, 2021 19:25
@stevegrunwell
Copy link
Contributor Author

Will do, I'm working through the pipeline to figure out why the autoloader isn't getting updated 🤔

…e-sensitive filesystems

Running locally on macOS, PHP can find `Database\Factories\TeamFactory` in `database/factories/TeamFactory.php`, but on case-sensitive systems (like Linux, used by the CI pipeline and hosts everywhere) PSR-4 becomes even more important.
@stevegrunwell stevegrunwell marked this pull request as ready for review October 28, 2021 19:50
@stevegrunwell
Copy link
Contributor Author

@driesvints figured it out: I had the following in composer.json:

"autoload-dev": {
    "psr-4": {
        // ...
        "Database\\": "database/"
    }
},

On a case-insensitive filesystem (like macOS) it wasn't a big deal, but PSR-4 would expect to find Database\Factories\TeamFactory in database/Factories/TeamFactory.php (capital "F"). PSR-4 failure on my part 😬

@taylorotwell taylorotwell merged commit ab149e8 into laravel:2.x Oct 28, 2021
@stevegrunwell stevegrunwell deleted the fix/team-permissions-when-missing-roles branch October 28, 2021 20:27
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.

None yet

3 participants