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

Setting permission shouldn't save a Role #39

Open
martin-ro opened this issue Aug 3, 2021 · 1 comment
Open

Setting permission shouldn't save a Role #39

martin-ro opened this issue Aug 3, 2021 · 1 comment
Labels
bug Something isn't working

Comments

@martin-ro
Copy link

$this->save();

Hi,

let's assume I extend your Role model to be able to use a Factory like so:

app/Models/Role.php

<?php

namespace App\Models;

use Illuminate\Database\Eloquent\Factories\HasFactory;

class Role extends \Pktharindu\NovaPermissions\Role
{
    use HasFactory;
}

database/factories/RoleFactory.php

<?php

namespace Database\Factories;

use App\Models\Role;
use Illuminate\Database\Eloquent\Factories\Factory;
use Illuminate\Support\Str;

class RoleFactory extends Factory
{
    /**
     * The name of the factory's corresponding model.
     *
     * @var string
     */
    protected $model = Role::class;

    /**
     * Define the model's default state.
     *
     * @return array
     */
    public function definition()
    {
        return [
            'name' => $slug = str_replace('.', '', $this->faker->text(rand(50, 100))),
            'slug' => Str::slug($slug),
            'permissions' => [],
        ];
    }
}

routes/web.php

Route::get('/test', function () {
    Artisan::call('migrate:fresh --force');

    $role = App\Models\Role::where('name', 'test')->first();
    dump($role);

    App\Models\Role::factory()->make(['name' => 'test']);

    $role = App\Models\Role::where('name', 'test')->firstOrFail();

    dump($role->name);
});

The expected behavior would be to fail here because make should not persist the record. If the Role does not exist at the time of setting permissions we should not just save the model?

@pktharindu
Copy link
Owner

Good catch!
This package basically uses the same models from Silvanite/brandenburg and I see the same issue (#22) there too.
I'll just keep an eye on this and try to PR a fix when I have some time.

PRs are welcome of-course 🙂

@pktharindu pktharindu added the bug Something isn't working label Sep 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants