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

Allow role resource configurable #418

Conversation

a21ns1g4ts
Copy link

Sometimes I need to change certain implementations without rewriting the entire plugin code. With this change, I'll be able to do that.
Currently, I need to modify the navigation label, but it's a translation value. To do that, I need to publish all the translation files, but I don't want to maintain this versioning in my own code. If you make this configurable, I can easily extend my own implementation.

Copy link

what-the-diff bot commented Sep 21, 2024

PR Summary

  • Introduction of a new configuration option
    A new flexible configuration option, role_resource, has been added to the filament-shield.php file. This will have an impact on how roles are assigned and managed within the application.

  • Update to the registration method
    The registration method within the FilamentShieldPlugin.php file has been updated. Previously, it used a hardcoded resource, Resources\RoleResource. Now, it uses the newly introduced role_resource configuration. This change provides more flexibility and customization to the registration process of the application.

@bezhanSalleh
Copy link
Owner

You can already do what you want by publishing the resource. Otherwise publish the lang files and only keep the languages you need delete the rest. Because it's not just the title and navigation translations there are column and icon translations as well.

@a21ns1g4ts
Copy link
Author

@bezhanSalleh Simply, you could allow us to modify the class implementation.

@bezhanSalleh
Copy link
Owner

Maybe my train of thought is on a whole different space right now. So could you elaborate on what is the difference between what you are purposing and publishing the resource?

@a21ns1g4ts
Copy link
Author

Allowing the main class to be swapped via configuration makes it easier to extend new features without re-implementing the plugin, while maintaining flexibility for future enhancements. As you can see, most plugins allow you to swap the resource class implementation. This is because it makes our implementation more specific without needing to suggest changes to the core code. 😄

@bezhanSalleh
Copy link
Owner

bezhanSalleh commented Oct 2, 2024

hmm... So you basically want to swap the role resource with your own implementation, is that what you are saying right?. you want the cli stuff but also away to use your own RoleResource instead. Now this distinction is very important because if it's just the question of overriding a couple of methods then, that would be totally different. if you are in filament discord then we can continue the discussion over there.

@a21ns1g4ts
Copy link
Author

@bezhanSalleh Alright. I need to change some implementations of the resource.

@bezhanSalleh
Copy link
Owner

to override a method let's take your example of the navigation label.

  1. create a resource and delete the pages and stuff or just a class in your specific panel's resource directory.
//ShieldOverrideResource.php
<?php

namespace App\Filament\Resources;

use BezhanSalleh\Shield\Resources\RoleResource;

class ShieldOverrideResource extends RoleResource
{
    public static function getNavigationLabel(): string
    {
        return 'Roles & Permissions';
    }
    
    // this is to avoid duplication
    public static function shouldRegisterNavigation(): bool
    {
        return true;
    }
}
  1. from filament-shield.php config disable the should_register_navigation. this is to avoid duplication
    'shield_resource' => [
        'should_register_navigation' => false,
        ...
  1. in your AppServiceProvider or any other service provider's boot() method bind the your own implementation.
use BezhanSalleh\Shield\Resources\RoleResource;

class AppServiceProvider extends ServiceProvider
{
   ...
   public function boot()
   {
        $this->app->bind(RoleResource::class, \App\Filament\Resources\ShieldOverrideResource::class);
        
        ...
   }
   ...

now you can override anything your heart desires and without re-writing the whole plugin as you put it.
hope this is what you want. let me know.

@a21ns1g4ts
Copy link
Author

a21ns1g4ts commented Oct 2, 2024

@bezhanSalleh If this works, it's very good. I didn't try it before because I thought you needed to use the app() in your implementation, but I think if it's like this, it's much better. I'll try and let you know if it worked.

@a21ns1g4ts
Copy link
Author

@bezhanSalleh I just tested it, and this doesn't work. That's because Filament doesn't instantiate the class internally through the injection container.

@bezhanSalleh
Copy link
Owner

bezhanSalleh commented Oct 6, 2024

@bezhanSalleh I just tested it, and this doesn't work. That's because Filament doesn't instantiate the class internally through the injection container.

FYI, you don't have to tag me, it's my repo so i get notified anyway.
Also the code i give you isn't a sample it's working code.
here ... https://github.com/bezhanSalleh/shield-demo
the above repo is the official filament-demo repo with shield installed.

It's a common way to override things in laravel so if it still doesn't work then you might have something somewhere faulty on your code base.

@a21ns1g4ts
Copy link
Author

Ok. Sorry to bother you, but this is very important to me. I tried doing what you said, but I didn't get any results. Can I contact you through Discord? Thank you very much!

@afsakar
Copy link

afsakar commented Oct 11, 2024

Sorry for bump this conversation but i have to do same thing. Did you find any solution? @a21ns1g4ts

For example, I want to add it to the App/Filament/Admin/Resources/Management file path. But the isResourcePublished method in the Utils class returns false in any case because I did not publish it to Filament/Resources. I think the main problem stems from here.

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.

3 participants