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

Adding OpenApi Schema Properties on Models #930

Closed
wants to merge 7 commits into from

Conversation

lion2486
Copy link

It's a very initial implementation, in order to work in the same way it should also be extended into the ReflectionClass package to support it from this site better.

@mfn
Copy link
Collaborator

mfn commented Apr 27, 2020

Please try to reduce (remove) all unrelated codestyle changes, hard to digest exactly what you're adding

kostis added 2 commits April 28, 2020 10:00
@lion2486
Copy link
Author

I removed the codestyle changes.

@mfn
Copy link
Collaborator

mfn commented Apr 28, 2020

As I'm not using OpenApi, it's hard to digest how this works.

Before having to write a test, can you share some examples how it works, what's the model look like before/after?

@lion2486
Copy link
Author

Here is the official OpenAPI 3 Schema Specification
http://spec.openapis.org/oas/v3.0.3#reference-object
Bellow I have a model example:

/**
 * App\User
 *
 * @OA\Schema ()
 * @mixin \Eloquent
 * @property int $id
 * @property string $name
 * @property string $email
 * @property string $password
 * @property string|null $remember_token
 * @property string $role
 * @OA\Property(property="id",type="int",description="")
 * @OA\Property(property="name",type="string",description="")
 * @OA\Property(property="email",type="string",description="")
 * @OA\Property(property="password",type="string",description="")
 * @OA\Property(property="role",type="string",description="")
 * @OA\Property(property="remember_token",type="string|null",description="")
 */

As you can see, I am adding the @OA\Schema () to the object and then I am adding the object properties as @OA\Propery. With this the composer package darkaonline/l5-swagger can detect that and define it as a OpenApi Schema with the following properties. The result specification can use this generated schema into controllers and other annotations. It also generates the complete OpenAPI specification in json and/or yml format (so you can view it in Swagger Editor).

@mfn
Copy link
Collaborator

mfn commented Apr 29, 2020

My gut feeling: this doesn't belong into this ide-helper ; rather it could be a plugin (*) for ide-helper, because that's really a outside-laravel-framework use-case and depends on a third party package.

(*) there is no dedicated "plugin" support, rather I'm meaning some kind of hack/integration/workaround

Just my 2c, I don't feel this "3rd party dependency special case" belongs into the library.

@mfn
Copy link
Collaborator

mfn commented Apr 29, 2020

But I would be all for it allowing other pacakges to easily hook into ide-helper to provide their additional logic

@lion2486
Copy link
Author

lion2486 commented May 2, 2020

Yeah, your perspective looks fair enough, but this plugin is already doing a great job and re-inventing the wheel in order to do an additional thing looks not smart. Having a hooking/attaching an other plugin into that and allow other plugin vendors to extend it's functionalities sounds a wonderful idea.
Is there a way for Feature suggestion in order to start a discussion on that part?

@mfn
Copy link
Collaborator

mfn commented May 4, 2020

Probably a good to ping @barryvdh what he things :-)

@barryvdh
Copy link
Owner

barryvdh commented May 4, 2020

I understand that it kinda makes sense if you're doing much of the same logic already. But not sure if it's in scope for this project. And OpenAPI doesn't always map directly to a model, does it?

@lion2486
Copy link
Author

lion2486 commented May 4, 2020

And OpenAPI doesn't always map directly to a model, does it?

Models are converted to OpenAPI Schemas and then they can used as $ref into the rest of the API-Spec (as request body, response, etc.)

@mfn
Copy link
Collaborator

mfn commented Jun 28, 2020

I firmly believe this shouldn't be in this package as it's very narrow focused.

Rather it can live in your code base or a package based on this, providing an adjusted command which hooks into the places where you need the modifications.

But as already mentioned, I would be all-in ensuring this is technically possible.

@barryvdh barryvdh closed this Jun 28, 2020
@mfn mfn mentioned this pull request Jun 28, 2020
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.

3 participants