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

Ability to define resourceful Action routes #295

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

CWAscend
Copy link

@CWAscend CWAscend commented Oct 16, 2024

I absolutely love this package, but the only thing I don't like is having to declare all of my Action routes individually, and losing the ability to define resourceful routes.

This PR attempts to achieve this, by leveraging the Route::resource() function and swaping out the Controller@action for the action class.

Example usage:

// By default, it will look for all actions in the App\Actions namespace
Route::actions('addresses');

// If we only want to to generate index/show routes
Route::actions('addresses')->only('index', 'show');

// If our action is not in the default namespace, we can specify the namespace
Route::actions('addresses', 'App\Actions\Addresses');

// Because we are leveraging the Route::resource() method, all other helpers are readily
// available for us to chain on to
Route::actions('addresses')->only('index')->middleware('signed');

The benefit of this is that it will force developers to have consistent routing AND consistent Action naming. Now the names of these are up for grabs, but I've gone for:

  • GetAddresses - this replaces Controller@index
  • ShowCreateAddress - this replaces Controller@create
  • ShowAddress - this replaces Controller@show
  • ShowEditAddress - this replaces Controller@edit
  • CreateAddress - this replaces Controller@store
  • UpdateAddress - this replaces Controller@update
  • DeleteAddress - this replaces Controller@destroy

Example - the following two route declarations:

Route::actions('addresses');
Route::actions('order-items');

produces the following routes:

image

As I mentioned above, if the Action is under a different namespace, we can specify that:

Route::actions('orders', 'App\Actions\Order')->only(['show', 'store']);

which produces:

image

@estin92
Copy link

estin92 commented Oct 16, 2024

+1

@Wulfheart
Copy link
Collaborator

Couldn't this also be its own package?

@Wulfheart
Copy link
Collaborator

Furthermore, could you please add tests?

@CWAscend
Copy link
Author

Couldn't this also be its own package?

@Wulfheart happy to wrap this up into it's own package if the community believes it'll be useful. Where else can you see this being useful away from the Laravel Actions package?

Furthermore, could you please add tests?

Of course, will add some test coverage for this 👍

@CWAscend
Copy link
Author

@Wulfheart @lorisleiva

I've added tests as requested, and have also added nesting/shallow nesting support.

For example

Route::actions('photos.comments');

will generate:

image

I've also added the ability (and test coverage) to allow developers to override the name of the Action class that is resolved in the routes, by using. For example, adding this to your RouteServiceProvider

use Illuminate\Support\Str;
use Lorisleiva\Actions\Routing\ActionResourceRegistrar;

ActionResourceRegistrar::resolveActionClassNameUsing(
    function ($resource, $method): ?string {
        return ucfirst($method)
            .ucfirst(Str::camel(str_replace('.', '-', $resource)))
            .'Action';
    }
);

If we return null from the above, it will fall back to whatever the default is defined as in this package.

@CWAscend
Copy link
Author

@Wulfheart @lorisleiva any thoughts?

@lorisleiva
Copy link
Owner

I don't mind adding this. I think it's well tested and could benefit some people but I find the name of the macro confusing.

We already have a way of registering routes within actions by doing Actions::registerRoutes(). Adding Routes::actions() to the API may confuse people as the distinction between the two isn't clear.

I'd rather have a slightly more lengthy macro name that better describes the operation such as Routes::resourceAsActions(), Routes::actionsResource() or even Actions::registerResourceRoutes().

@CWAscend
Copy link
Author

CWAscend commented Nov 7, 2024

I don't mind adding this. I think it's well tested and could benefit some people but I find the name of the macro confusing.

We already have a way of registering routes within actions by doing Actions::registerRoutes(). Adding Routes::actions() to the API may confuse people as the distinction between the two isn't clear.

I'd rather have a slightly more lengthy macro name that better describes the operation such as Routes::resourceAsActions(), Routes::actionsResource() or even Actions::registerResourceRoutes().

Route::resourceActions() makes sense, I much prefer that to be fair! How do you envisage Actions::registerResourceRoutes() working? I'm happy to try and implement something here to keep the resource registration service provider driven, although I'm not sure if there is a clean way of doing it.

How would we define the individual namespaces if Actions are not in the app/Actions dir, how could we specify

->only('index', 'store')

as an example, and how can we tackle nesting/shallow nesting?

I personally think we should keep resourceful actions as solely defined in route files for the above reasons

Copy link
Owner

@lorisleiva lorisleiva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy to go with Route::resourceActions.

}

if (empty($actionClass)) {
$actionClass = match ($method) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can $method be a value that isn't listed in the match values?

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.

4 participants