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 12 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?

Copy link
Author

@CWAscend CWAscend Feb 2, 2025

Choose a reason for hiding this comment

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

I've changed this to Route::resourceActions as requested :)

For this comment, I've replicated what is in the parent class:

image

So my answer your question is no - there's also no method that allows change the $resourceDefaults array, unless the developer extends this class and forces a change that way.

In this case, if they need the Action class names to be resolved in a different way to the default functionality provided, then can customise it by leveraging this in their RouteServiceProvider:

ActionResourceRegistrar::resolveActionClassNameUsing(
    function ($resource, $method): ?string {
        // Add their custom functionality here
    }
);

So lets say they extended the ResourceRegistrar and appended a 'restore' to the resource defaults:

ActionResourceRegistrar::resolveActionClassNameUsing(
    function ($resource, $method): ?string {
        return match ($method) {
            'restore' => 'Restore'.ucfirst($resource),
            default => null        
        }
    }
);

@CWAscend
Copy link
Author

Keen to get this over the line in the coming days, apologies for the delay - I was out of the country and off the tools for a while towards the end of last year!

@lorisleiva
Copy link
Owner

@CWAscend Is this ready for another review? No worries if not but please ping me when it's ready so I don't miss it.

@CWAscend
Copy link
Author

@CWAscend Is this ready for another review? No worries if not but please ping me when it's ready so I don't miss it.

@lorisleiva It is now! I've been sitting on the functionality that allows you to override the default naming conventions, and have amended it to allow for more granular control. I think the change is easier to follow than what was there before.

Let me know what you think!

@CWAscend CWAscend requested a review from lorisleiva February 10, 2025 00:22
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