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

[Feature] CrudRoute Abstraction for extensibility #230

Merged
merged 5 commits into from
Nov 13, 2016

Conversation

OwenMelbz
Copy link
Contributor

@OwenMelbz OwenMelbz commented Nov 11, 2016

Instead of having a static CRUD::resource within the service provider I’ve abstracted it out into CrudRouter.

This then returns from the original resource allowing people to add extra closures, and will permit us to add extra routes to resources.

e.g

CRUD::resource(‘teams’, ‘Admin\TeamCrudController’)->withUniqueCheck(); // more of these are added via traits

//or

CRUD::resource(‘teams’, ‘Admin\TeamCrudController’)->with(function(){
    // I can be used to run call backs, or add extra routes to this resource
});

//or

CRUD::resource(‘teams’, ‘Admin\TeamCrudController’)->with(['uniqueCheck','allImages']);

//or

CRUD::resource(‘teams’, ‘Admin\TeamCrudController’)->with('uniqueCheck');

The first example is what I imagine to be the most used method, it will allow us to have lighter weight routes, then extend them when desired. This would allow us to implement things like the unicity checks etc.

As its backwards compatible, would be nice to get this in asap :P especially as we're using it currently :D

OwenMelbz and others added 2 commits November 11, 2016 19:00
abstracted the crud router away from the service provider, so it can be
extended and chained, more laravelly!
[ci skip] [skip ci]
@tabacitu
Copy link
Member

This. Looks. AWESOME!

@tabacitu
Copy link
Member

Just tried it out, it looks and works great. Awesome work.

  1. I did rename some methods to make it more clear what the actually do, check it out here 03392d1, please. Don't know if I chose the best names, though. Since this is a non-breaking feature, I'll go ahead and merge it anyway and we'll leave it undocumented until we settle the names.

  2. One thing that really excites me about this feature is that we can provide developers with a cleaner way to add their extra routes for a CrudController in one place. Something like:

CRUD::resource(‘teams’, ‘Admin\TeamCrudController’)->with(function(){
    // I can be used to run call backs, or add extra routes to this resource
    Route::get('teams/ajax-name-options', 'Admin\TeamCrudController@nameOptions');
    Route::get('teams/ajax-category-options', 'Admin\TeamCrudController@categoryOptions');
});

But I tried it and it didn't work, I got 403 error. I don't get it. Checkout out the methods again. It should work. Checked out php artisan route:list, the routes are there, under the right middleware.
screen shot 2016-11-13 at 09 15 20

Any idea? Does it work for you?

Again, really awesome work man, this is such a good PR :-)

@tabacitu tabacitu merged commit ded3e7a into master Nov 13, 2016
@tabacitu
Copy link
Member

Ok I think I got it:

screen shot 2016-11-13 at 09 23 18

Damn...

@tabacitu
Copy link
Member

Ok, I fixed it by placing the CRUD::resource() in the destructor instead of the constructor. Works fine now.

Sorry for the verbosity. Only need you to confirm you're ok with the method names or propose better ones. Cheers!

@OwenMelbz
Copy link
Contributor Author

Oh thats very odd! I'd literally been using it and hadn't experienced that! super weird!

But yeah it means we could have withFilters, withSearch etc etc as optional, rather than adding all the extra ones by default

One thing I wanted it to do was act like a route::group, so if you had like

CRUD::resource(‘teams’, ‘Admin\TeamCrudController’)->with(function(){
    // I can be used to run call backs, or add extra routes to this resource
    Route::get('ajax-name-options', 'Admin\TeamCrudController@nameOptions');
    Route::get('ajax-category-options', 'Admin\TeamCrudController@categoryOptions');
});

it would prefix the route with teams/ajax-name-options

But wouldn't figure out anyway to do that without creating custom CRUD::get etc.

Regards to the renaming yes thats fine! they were legacy names actually, previously I had it setup differently and just renamed it all for the PR and missed them! so good spot!

I also really like the idea of the without I guess to get this working, we'd just have to loop through all the default routes first, pushing them in if they're not defined!

@tabacitu tabacitu deleted the chainable-resources branch January 8, 2017 11:52
@tabacitu
Copy link
Member

tabacitu commented Jan 8, 2017

Uuuu... ->without() sounds awesome, I agree.

This was referenced Apr 2, 2020
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.

2 participants