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

php artisan backpack:extend-permissionmanager #23

Closed
tabacitu opened this issue Sep 29, 2016 · 10 comments
Closed

php artisan backpack:extend-permissionmanager #23

tabacitu opened this issue Sep 29, 2016 · 10 comments

Comments

@tabacitu
Copy link
Member

A command that makes overwriting default functionality easier.

An artisan command that creates blank controllers, requests and models. Those classes:

  • would be generated inside the respective project folders;
  • would extend the classes in Backpack;
  • would have the proper namespace;

So that the developer would only need to write the logic for the method they want to overwrite.

@b8ne
Copy link

b8ne commented Oct 2, 2016

Hey @tabacitu , when I forked and did this for my project I just add this to PermissionManagerServiceProvider:
$this->publishes([__DIR__.'/app/Http/Controllers' => app_path('Http/Controllers/Permissions')], 'permission-controllers');
$this->publishes([__DIR__.'/app/Http/Requests' => app_path('Http/Requests/Permissions')], 'permission-requests');
$this->publishes([__DIR__.'/app/Models' => app_path('Models/Permissions')], 'permission-models');

I altered the default controllers and requests to have the right namespaces though and also altered the route group namespace, how would you setup this to cater for both cases?
If its simple im happy to alter and tidy up what ive done and submit a PR.

@tabacitu
Copy link
Member Author

tabacitu commented Oct 2, 2016

I guess the proper procedure would be very similar, but automated. So the commands wouldn't be defined as "publish" commands, as they would need to do some extra stuff too. I think the best place to put a command like this would be inside src\Console\Commands\ExtendPermissionManagerCommand.php, like we do in Backpack\Generators, to keep the Laravel convention. That command can then be easily registered inside PermissionManagerServiceProvider.

Then the ExtendPermissionManagerCommand would do things very similar to how Backpack\Generator does with the Controller/Model/Request stubs:

  • publish some blank controllers, requests, models, that extend the ones in Backpack; I personally would have all their methods there, calling their parents, so it would encourage people do things before or after the parent call, if possible, not overwrite the entire functionality; this way they'd also benefit from updates;
  • replace Backpack\ with AppNamespace\ everywhere inside the copied stubs, where AppNamespace can be determined this way (we can replace it the same way generators do with table name);
  • give success/error messages to the developer in the command line (x published, y published, z published);
  • remind the developer in the command line that he now needs to setup the routes that point to these new controllers and give him the code so he can copy-paste:
CRUD::resource('permission', 'PermissionCrudController');
CRUD::resource('role', 'RoleCrudController');
CRUD::resource('user', 'UserCrudController');

To sum up, honestly, I think the code is pretty different. It's more work to automate it, so I don't think merging with your fork makes sense right now. I would be very very happy to merge, however, if you did it this automated way :-) It would be a big help for me personally and I think, for hundreds/thousands others that use the package.

Cheers!

@datune
Copy link

datune commented Dec 25, 2016

Since I need this exact functionality I will look into this tomorrow, fork it and create a PR once it works. So basically, feel free to assign it to me ;-)

Simply extending the BP Role and Permission Class within my own and using the config file to adjust the table names has already gotten me pretty far. Going to need a bit more time to look into this properly.

@kiddtang
Copy link
Contributor

I wanted to extend the Controller, Request & Model.

However, The CRUD Controller throw me the exception for the extended class from original PermissionCrudRequest

Declaration of App\Http\Controllers\Admin\PermissionCrudController::store(App\Http\Requests\PermissionRequest $request) should be compatible with Backpack\PermissionManager\app\Http\Controllers\PermissionCrudController::store(Backpack\PermissionManager\app\Http\Requests\PermissionCrudRequest $request)

@OliverZiegler
Copy link
Contributor

@kiddtang your PermissionRequest should inherit from the Backpack PermissionCrudRequest to fix this.

@kiddtang
Copy link
Contributor

kiddtang commented Dec 3, 2017

@OliverZiegler I did that but the Warning still there...

I am overriding authorize() method for my customized guard

namespace App\Http\Requests;

use Backpack\PermissionManager\app\Http\Requests\PermissionCrudRequest as BackpackPermissionCrudRequest;
use Illuminate\Foundation\Http\FormRequest;

class PermissionRequest extends BackpackPermissionCrudRequest
{
    /**
     * Determine if the user is authorized to make this request.
     *
     * @return bool
     */
    public function authorize()
    {
        return \Auth::guard('admin')->check();
    }

}

My Controller then use the PermissionRequest

namespace App\Http\Controllers\Admin;

use Backpack\PermissionManager\app\Http\Controllers\PermissionCrudController as BackpackPermissionCrudController;

use App\Http\Requests\PermissionRequest as StoreRequest;
use App\Http\Requests\PermissionRequest as UpdateRequest;

class PermissionCrudController extends BackpackPermissionCrudController
{
    public function __construct()
    {
        parent::__construct();
    }

    public function store(StoreRequest $request)
    {
        return parent::storeCrud();
    }

    public function update(UpdateRequest $request)
    {
        return parent::updateCrud();
    }
}

@abdelrahmanahmed
Copy link

@kiddtang Have you solved your issue , because i am facing the exact same error in my project..

@kiddtang
Copy link
Contributor

@abdelrahmanahmed
I modify the package source file PermissionCrudController.php

    public function __store(StoreRequest $request)
    {
        return parent::storeCrud();
    }

    public function __update(UpdateRequest $request)
    {
        return parent::updateCrud();
    }

So my function could override the default behavior. Hope it could help you.
I am not sure it is the right way or not, but at least there's no issue on my project so far.

@claudsonm
Copy link

I'm also facing this issue. But I don't like the idea to manually overwrite the vendor files. What I did was to write the validations inline, inside the extended controller.

You can accomplish that by just writing inside the store and update methods:

$request->validate([
    'name' => [
        'required',
        'min:10',
    ],
]);

Of course, you can extract your validation to another method and call it instead of writing this twice.

@tabacitu
Copy link
Member Author

Hey everyone! I no longer think this is worth doing. I think it's one of those cases where clear documentation eliminates the need for a feature. And I think this section provides clear instructions on how to customize any class this package provides. Plus, we shouldn't make it too easy for people to extend this and customize... if they do that without knowing what they're doing... that's a recipe for disaster 😔

So I'm going to close the issue. This wasn't a priority anyway, so it's unlikely we would get to it this year either. Better closed than "never done".

Let me know if you disagree. I'm wrong at least once a day, like everybody else 😀

Cheers!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants