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

[5.6] Adds --api flag to make:controller #22996

Merged
merged 9 commits into from
Feb 3, 2018
Merged

[5.6] Adds --api flag to make:controller #22996

merged 9 commits into from
Feb 3, 2018

Conversation

OzairP
Copy link
Contributor

@OzairP OzairP commented Feb 2, 2018

👋

Synopsis

This PR allows developers to add the --api flag when running the make:controller --resource command in Artisan. This is useful for creating API resource routes via Route::apiResource(...).

Reasoning

Currently the --resource flag adds CRUD methods plus two additional methods for viewing forms for creating and updating. The --api flag along with the --resource flag will remove the two additional methods which are not needed for REST end points.

This is a QoL PR so developers won't have to create resource controllers, find and open the file, then find the appropriate methods to remove.

Breaking Changes

This PR introduces no breaking changes. This will only affect future code generations should the invoker decide to opt in with the --api flag.

Testing

This PR also includes tests for

  • make:controller (plain)
  • make:controller --resource
  • make:controller --resource --parent=...
  • make:controller --resource --api
  • make:controller --resource --api --parent=...

I believe this is the firsts suite of tests to test Artisans code generation, so this somewhat sets a standard on how they should be tested. If anyone have any suggestions on how to improve the assertions please let me know 😄 .

(Extends from #22920)

@stefanoruth
Copy link
Contributor

How about using the --api without --resource can that be done too?

@Miguel-Serejo
Copy link

I suppose --api would imply --resource, so using both should be unnecessary.

@taylorotwell
Copy link
Member

Can you remove all of those assertions files and such. It just looks like a bunch of duplicated stuff - I'm not sure how valuable that actually is.

@OzairP
Copy link
Contributor Author

OzairP commented Feb 2, 2018

@stefanoruth @36864 I agree, I would prefer to have done --resource=api, but I suppose I was following the current track of how flags are being added, like --parent.

@taylorotwell I agree that the assertion files are code duplication. You did ask for me to add some tests from my last PR so this is the only way I saw of writing the integration test cleanly. Would you like me to remove all the tests or figure out a different way of testing it?

@taylorotwell
Copy link
Member

Just remove all the tests and I'll work on it.

@OzairP
Copy link
Contributor Author

OzairP commented Feb 3, 2018

I'm looking forward to see your strategy on testing this.

@taylorotwell taylorotwell merged commit 8628885 into laravel:5.6 Feb 3, 2018
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