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

[8.x] New flag --requests -R to make:controller and make:model Commands #39120

Merged
merged 11 commits into from
Nov 5, 2021
Merged

[8.x] New flag --requests -R to make:controller and make:model Commands #39120

merged 11 commits into from
Nov 5, 2021

Conversation

PovilasKorop
Copy link
Contributor

@PovilasKorop PovilasKorop commented Oct 6, 2021

This PR adds a new flag to php artisan make:controller command, which creates FormRequest class and uses it immediately in the generated controller.

Problem

How I personally use Resourceful Controllers with Form Requests:

  1. php artisan make:controller UserController --resource
  2. php artisan make:request StoreUserRequest
  3. php artisan make:request UpdateUserRequest
  4. Go to the UserController and manually replace Illuminate\Http\Request with those new FormRequest classes

Why couldn't we generate it all at once?

This problem resonated with my Twitter audience, with 180+ likes on this Tweet.


My Proposed Solution (updated October 7)

Now, when you run php artisan make:controller UserController --resource --model=User --requests, it will generate two FormRequest classes: StoreUserRequest and UpdateUserRequest, and will replace them in the Controller.

A short version of --requests flag is -R, not to mix with -r for resources.

It is done by reusing the make:request command and calling it with appropriate class name.

This functionality works only together with --model flag, otherwise it would be impossible/hard to guess the name for the FormRequest classes.

Also, it works one "layer" above in the Model: php artisan make:model Project -mcrR would generate StoreProjectRequest and UpdateProjectRequest


Breaking changes

It shouldn't break any existing functionality, it's just a new flag. If that flag isn't used, it all falls back to the old default values.


Any other suggestions for improvement are welcome.

@GrahamCampbell GrahamCampbell changed the title New flags --request --requests to make:controller Command [8.x] New flags --request --requests to make:controller Command Oct 6, 2021
@tontonsb
Copy link
Contributor

tontonsb commented Oct 6, 2021

For me the "make everything" method is make:model. I usually do php artisan make:model -mrc User and that's where a request flag would be useful as well. It would also solve your "--model is a must" problem.

I am not suggesting to remove anything, but that proxying the new flags from make:model to make:controller would make this more useful. At least in my workflow.

@PovilasKorop
Copy link
Contributor Author

Thanks @tontonsb

For me the "make everything" method is make:model. I usually do php artisan make:model -mrc User and that's where a request flag would be useful as well. It would also solve your "--model is a must" problem.

I thought about it, but to me, it feels like opening a deeper "rabbit hole / inception".
Not sure, maybe someone will comment, should I add --request(s) to the model as well, then? But there's no "short" for -mrc then cause letter r is already taken for resource controllers

So now sure how elegant it would be php artisan make:model Project -mcr --requests

Opinions?

@taylorotwell
Copy link
Member

taylorotwell commented Oct 6, 2021

I think @tontonsb has a pretty good point but I also do not have a good solution for the shortcut... only thing I can think of is h for "HTTP Requests"... 🤷‍♂️ but that isn't great.

@caugner
Copy link
Contributor

caugner commented Oct 6, 2021

So now sure how elegant it would be php artisan make:model Project -mcr --requests

Maybe -x for --requests would be a viable alternative (as in curl -X)? Or even -R.

PS: If we introduced uppercase letters, -A could become a shorthand for --api (or -C for -c --api).

@taylorotwell
Copy link
Member

I think @caugner's suggestion of capital R is not bad.

@PovilasKorop
Copy link
Contributor Author

@caugner great suggestion about -R, I agree, will implement tomorrow.

@caugner
Copy link
Contributor

caugner commented Oct 6, 2021

Just to add two more ideas:

  • An interactive mode (-i), especially for beginners and devs that don't know yet what they want.
  • A wizard mode (-w) that determines the choices based on which items already exist and which namespaces the project already uses.

@taylorotwell
Copy link
Member

That might be a bit out of scope on this PR 😅

@PovilasKorop
Copy link
Contributor Author

@caugner I agree with @taylorotwell - I guess I will create a full wizard with bells and whistles in the future :)

@tontonsb
Copy link
Contributor

tontonsb commented Oct 6, 2021

PS: If we introduced uppercase letters, -A could become a shorthand for --api (or -C for -c --api).

It's not an "if". We already have -V for version and -v for verbosity.

However, --request and --requests sounds like something that would correspond to lowercase and uppercase of the same letter. Or are you only shorthanding one of them? Which one then?

@taylorotwell
Copy link
Member

@PovilasKorop I personally only think you should include the requests version and not the single request. I typically would never create a single request to handle both create and update.


$namespacedRequests = $namespace.'\\'.$storeRequestClass.';';
if ($storeRequestClass != $updateRequestClass) {
$namespacedRequests .= "\r\nuse ".$namespace.'\\'.$updateRequestClass.';';
Copy link
Contributor

Choose a reason for hiding this comment

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

The use of \r here is not a UNIX standard linebreak.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@TomHAnderson good notice, fixed in these:

@TomHAnderson
Copy link
Contributor

I think this PR should include a reference to a complimentary PR in the docs.

@PovilasKorop
Copy link
Contributor Author

@PovilasKorop I personally only think you should include the requests version and not the single request. I typically would never create a single request to handle both create and update.

Yup, agreed, me too. Removed the singular --request.

Also, pushed the changes for make:model --requests flag like @tontonsb suggested, and for accepting -R as a short version.

@TomHAnderson will submit a PR to the docs then this PR is fully merged, then I will know which flag to put where in the docs.

@PovilasKorop PovilasKorop changed the title [8.x] New flags --request --requests to make:controller Command [8.x] New flag --requests -R to make:controller and make:model Commands Oct 7, 2021
@SjorsO
Copy link
Contributor

SjorsO commented Oct 7, 2021

Just to add two more ideas:

  • An interactive mode (-i), especially for beginners and devs that don't know yet what they want.
  • A wizard mode (-w) that determines the choices based on which items already exist and which namespaces the project already uses.

Slightly related, I tweeted about a proof of concept for artisan make:migration --interactive a while back:

gif.mp4

I think a similar interactive mode for make:model would definitely improve the developer experience.

@PovilasKorop
Copy link
Contributor Author

@taylorotwell sorry for pushing but is there any reason why this PR wasn't merged/released in 20 days? Anything I still need to improve here?

@taylorotwell
Copy link
Member

I have mainly been hesitating because this is somewhat related to and close to functionality I was considering building in a laravel/concepts package that was a generator building out an entire "concept"'s files in your application. Typically, related to a model.

So, php artisan make:concept Invoice generates controllers, requests, tests, events, and so on for that concept, and it also generated store and update requests like this PR does.

I've been debating if I want to pursue that package or not so that has caused the hesitation here.

@PovilasKorop
Copy link
Contributor Author

@taylorotwell ok feel free to take over, this would be similar to "full wizard with bells and whistles in the future" I was referring to, earlier.

To be honest, there are a lot of CRUD generator packages/services on the market already, so that functionality is largely covered by those packages. But I think the community would appreciate something like that inside the framework itself.

Just keep in mind the potential overhead of all the potential combinations of those files that may have their content depending on each other, at least in my experience the scope can grow quite quickly. But if you're up for the challenge, go for it, and let me know if I can contribute.

@DarkGhostHunter
Copy link
Contributor

I have mainly been hesitating because this is somewhat related to and close to functionality I was considering building in a laravel/concepts package that was a generator building out an entire "concept"'s files in your application. Typically, related to a model.

So, php artisan make:concept Invoice generates controllers, requests, tests, events, and so on for that concept, and it also generated store and update requests like this PR does.

I've been debating if I want to pursue that package or not so that has caused the hesitation here.

I vote for this solution so I can stop maintaining Larawiz.

@dragonfly4
Copy link

dragonfly4 commented Nov 18, 2021

@PovilasKorop Thanks for this PR!

Would you consider a follow-up release to create the Request files in the correct subfolder? Saves time to move 2 files and edit 3 of them. Or is that blocked by default Laravel behaviour?

Simple example

php artisan make:model "Accounting\CustomerAddress" -crmfR

This will create a new model under App\Models\Accounting
The Controller is created under App\Http\Controllers
The Requests are created under App\Http\Requests

@PovilasKorop
Copy link
Contributor Author

@dragonfly4 I thought about it but had a doubt if everyone would want to create subfolders for the requests exactly as for models.

Also, it would require quite a lot of work to make it properly, for now I moved on to other work and don't have that much time, sorry

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.

None yet

9 participants