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
Use default value of PHP properties to Swagger doc #2386
Use default value of PHP properties to Swagger doc #2386
Conversation
d30616f
to
9fa9635
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice improvement! I made some design comments but I like the idea!
src/Metadata/Property/Factory/DefaultPropertyMetadataFactory.php
Outdated
Show resolved
Hide resolved
src/Metadata/Property/Factory/DefaultPropertyMetadataFactory.php
Outdated
Show resolved
Hide resolved
src/Metadata/Property/Factory/DefaultPropertyMetadataFactory.php
Outdated
Show resolved
Hide resolved
src/Metadata/Property/Factory/DefaultPropertyMetadataFactory.php
Outdated
Show resolved
Hide resolved
@dunglas Won't 2.3 be a better branch target for this? |
@soullivaneuh no because it's a new feature. |
e2e37b3
to
c2a32e7
Compare
@dunglas PR updated to introduce the |
@@ -74,6 +74,10 @@ | |||
<argument type="service" id="api_platform.metadata.property.metadata_factory.cached.inner" /> | |||
</service> | |||
|
|||
<service id="api_platform.metadata.property.metadata_factory.default_property" decorates="api_platform.metadata.property.metadata_factory" decoration-priority="30" class="ApiPlatform\Core\Metadata\Property\Factory\DefaultPropertyMetadataFactory" public="false"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
decoration-priority="30"
is correct ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@meyerbaptiste done |
4f541a1
to
5ff77d5
Compare
5ff77d5
to
78e5a0a
Compare
Should we also use the Doctrine default value in https://github.com/api-platform/core/blob/master/src/Bridge/Doctrine/Orm/Metadata/Property/DoctrineOrmPropertyMetadataFactory.php? |
fc50d2d
to
5395ec6
Compare
I also added the Doctrine default value but I'm not sure it's a good idea because with postgres, Doctrine insert a null value for the column, and there is the error I can use Doctrine default value as example value but a doctrine annotation |
Not sure I got that, what is behind this insert? About the second point, thoughts @api-platform/core-team ? |
Given an entity with Doctrine default value
When I send the request
To have the default value inserted with postgres, the query should be So, it is maybe better to use the Doctrine default value as example instead of default property metadata and change https://github.com/api-platform/core/pull/2386/files#diff-fbaaae4f33f38c0ec0e318994cb386faR82 |
👍 if you can fix this and add a test case for it |
@aaa2000 you can fix et add test please for merge this pull request ? |
I can't fix, the fix should be done in doctrine. The doctrine ORM doesn't take into account the |
Okay nice, we'll open an issue to track this. I the meantime I'm 👍 to merge this. |
Add default and example information to swagger context if php properties have default values so the exemple documentation section use these values. Close api-platform#2363
5395ec6
to
a83c118
Compare
Thanks for the work @aaa2000 |
It's not a bug. That's exactly what it's intended for, the default value at the database level. It'd be a bug if Doctrine ORM actually tries to use it. 😉 |
Add default and example information to swagger context if php properties
have default values so the exemple documentation section use these values.
With a class
The swagger documentation displays