Laravel-apidoc-generator: [3.0] Replace FormRequest parsing with explicit parameter descriptions

[shalvah]

being able to parse Form Requests and generate parameter listing was a nice feature, as it hooked into a framework feature, and it allowed devs to declare request parameters only once. But we keep running into more and more issues with this, such as #247, #162, #294, #285, #253, and more. In addition, this parsing process involves a lot of code gymnastics and is pretty non-deterministic. (see #333 - it fails unless rules are ordered in a certain way). I don't think it's worth it trying to maintain this, especially as Laravel's validation system keeps getting more and more robust.

And from an SRP perspective, it's arguable whether we should be using FromRequests to generate doc. When I come to the "description" field for a parameter, what I really want to see is the business rule description (for example, "this is the ID of the latest transaction to fetch", rather than "This must be numeric"). Achieving the former with FormRequests is difficult, and rightfully so. FormRequests are for validation, not information or user guides.

In line with this, version 3 will no longer parse form requests. We'll add a new annotation (@bodyParam ) that will replace this. Something like this:

/**
 * @bodyParam user_id string required The ID of the user
 * @bodyParam room string required The ID of the room to ban the user from
 * @bodyParam length int The length of the ban, in days. Defaults to 7.
 */
public function banUser() 
{
  // ...
}

Admittedly, it's limited (can't nest array/object params).. We'll see if we can improve on that.
We're also open to alternate means that will make life easier for developers.

[mykeels]

Hey @shalvah ...

For Objects, something like

/**
 * @bodyParam room object required The room object
 * @bodyParam room.id int required The ID of the room to ban the user from
 * @bodyParam room.hostel_id int The hostel ID of the room to ban from
 */

should be descriptive enough, and ...

For Arrays, a structure like

/**
 * @bodyParam rooms array required A list of room objects
 * @bodyParam rooms[0] object required The first room object
 * @bodyParam rooms[0].id int required The ID of the first room to ban the user from
 */

could work.

[shalvah]

Completed in #362 (no support for arrays or objects yet, though)

[Punksolid]

The reason for some (or a lot of) people to use this package its because the easiness to use without making explicit everything, I agree that generating dynamically has a lot of bugs but also on simple request its incredibly fast and always up to date. There should be a way to opt-out the defaults FormRequest parsing to the explicit @bodyParam.
If not we could end in another long, complex and time-consuming swagger apidoc documentation.

[shalvah]

@Punksolid thanks for contributing to this discussion.

I agree that generating dynamically has a lot of bugs but also on simple request its incredibly fast and always up to date.

I only agree with the "always up-to-date" part, but there's nothing we can do about that right now.

Using explicit annotations forces you to think and write descriptions for your parameters. Admittedly, yeah, it's easy for the code to change and a dev to forget to update the annotations, but we can't help that.

And, no, using @bodyParam is nowhere near Swagger's level of tedium. With Swagger you need to define objects for every little thing and write a whole bunch of boilerplate we don't do here.

A good percentage of Laravel devs don't necessarily use FormRequests by default. The first time I used this tool, I was taken aback to learn it needed FormRequests, as I had been planning to use the $request->validate() method. And I've seen other people mention this in the issues. So it's not as "universal" a problem as you might think.

Here's my take:

1. I still disagree with the concept of using FormRequests to generate parameters. I still think they're for validation, not description.
2. I'm unwilling to keep putting in the effort and time writing code to support them. When I joined this project, my biggest headache was dealing with FormRequest parsing. I've spent several hours thinking of how to make it foolproof. Now I've given up 😀

However:

1. I'll accept PRs adding support for this.
2. But I won't make any bugfixes to FormRequest support, though.
3. I'll also mark it as "not recommended" in the documentation, and I'll respond to any issues raised with "use @bodyParam "(or mention the person (s) who implemented it).

Does that work? 🙂

[Punksolid]

Well I guess that if I'm not going to contribute with the project for now I can't expect features, I was just adding that FormRequest validations are useful in a lot of cases and that I consider faster to make a FormRequest, since with them we don't document our code but "code our documentation".
I just hope to not become another swagger.

[shalvah]

I appreciate you sharing your concerns @Punksolid. Let's try things out the new way and see how it goes. Rest assured, I dislike Swagger as much as you do, and I won't let us go there. 😅😅

[daraul]

I'd suggest adding this back, with some changes:

1. Only very basic support:
   If laravel-apidoc-generator encounters any issues while parsing a Request class, then stop, and spit out nothing for that endpoint's validation (maybe put a warning in the generated docs).
2. If the Request class presents no issues, use it's doc-comment as usual pre-3.0
   I'm not sure how comments for individual attributes were added, or how it'd figure out whether a parameter is expected as a queryParam or bodyParam (laravel doesn't seem to care)
3. If there's a @bodyParam or @queryparam defined for any attribute, then skip all this and use that instead
   The reasoning here is that if the developer took the time to write this out, essentially doing double-work, there must be a good reason for it. Maybe they're validation is too complex for the tool to handle
   Maybe even extend this to replace the description for some parameter with whatever is found in @bodyParam / @queryParam.

[shalvah]

Thanks for your suggestion, but there are good reasons why they won't be added back. Ignoring the amount of work involved in implementing FormRequests (work I don't have time for), a major blocker is the work in maintaining it. They lead to an avalanche of issues (just look at https://github.com/mpociot/laravel-apidoc-generator/issues?utf8=%E2%9C%93&q=is%3Aissue+formrequest+). When we provided "full" support for FormRequests, people were opening issues about them every other week; how much more if we decide to supply only "minimal" support as you suggest? There'll be lots of "why isn't X working?" all over. And then, people will start requesting support for other forms of Laravel request validators all over again. Long story short: I am unwilling and unable to bear the maintenance burden of that.

Also, FormRequests were meant for validation, not documentation. They can't distinguish between body and query parameters or add descriptions or examples without polluting your code with fake rules that could be confusing to a new team member.

The only solutions I can think of are:

1. Someone forking this library into theirs and adding FormRequest support
2. Moving to some sort of plugin architecture, so you can create a plugin that pulls from FormRequests, if you like. This is probably best, and I'll consider this for version 4, but I can't make any guarantees.

[daraul]

Moving to a plug-in architecture seems like the best bet. Then the plug-in
maintainer can deal with the those headaches you mentioned lol

On Wed, Aug 28, 2019, 8:14 PM Shalvah [e-mail] wrote:

Thanks for your suggestion, but there are good reasons why they won't be
added back. Ignoring the amount of work involved in implementing
FormRequests (work I don't have time for), a major blocker is the work in
maintaining it. They lead to an avalanche of issues (just look at
https://github.com/mpociot/laravel-apidoc-generator/issues?utf8=%E2%9C%93&q=is%3Aissue+formrequest+).
When we provided "full" support for FormRequests, people were opening
issues about them every other week; how much more if we decide to supply
only "minimal" support as you suggest? There'll be lots of "why isn't X
working?" all over. And then, people will start requesting support for
other forms of Laravel request validators all over again. Long story short:
I am unwilling and unable to bear the maintenance burden of that.

Also, FormRequests were meant for validation, not documentation. They
can't distinguish between body and query parameters or add descriptions or
examples without polluting your code with fake rules that could be
confusing to a new team member.

The only solutions I can think of are:

1. Someone forking this library into theirs and adding FormRequest
   support
2. Moving to some sort of plugin architecture, so you can create a
   plugin that pulls from FormRequests, if you like. This is probably best,
   and I'll consider this for version 4, but I can't make any guarantees.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/mpociot/laravel-apidoc-generator/issues/343?email_source=notifications&email_token=AAST6D32HVDJL7VIBROPXG3QG4IETA5CNFSM4FZOPGD2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD5M2WFY#issuecomment-525970199,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAST6DZVP6LL6LRJ3AFPGVDQG4IETANCNFSM4FZOPGDQ
.

[shalvah]

First PR for a plugin architecture cc @daraul and anyone interested. Any feedback would be appreciated.

[shalvah]

PR: https://github.com/mpociot/laravel-apidoc-generator/pull/570

[daraul]

Looks good! My only question is whether or not the __invoke() method would receive the arguments passed to the controller method as well. Would that be in the $context parameter?

[shalvah]

You can get controller parameters from the $method. It's an instance of
ReflectionMethod.https://www.php.net/manual/en/class.reflectionmethod.php

On Tue, 3 Sep 2019, 00:39 Daraul Harris, [e-mail] wrote:

Looks good! My only question is whether or not the __invoke() method
would receive the arguments passed to the controller method as well. Would
that be in the $context parameter?

—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
https://github.com/mpociot/laravel-apidoc-generator/issues/343?email_source=notifications&email_token=ADNSD4J6E4EW3WR5VA6XGZLQHWP3VA5CNFSM4FZOPGD2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD5WVDYI#issuecomment-527258081,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ADNSD4JTVY5PARIOAW7ZZALQHWP3VANCNFSM4FZOPGDQ
.

[shalvah]

FormRequest rules() support is back in Scribe.😅