fix: do not mark properties with default as required #2181
Conversation
| @@ -39,6 +39,11 @@ protected function setNullableProperty(Type $type, OA\Schema $property, ?OA\Sche | |||
| return; | |||
| } | |||
|
|
|||
| // Do not describe default values as required | |||
| if (Generator::UNDEFINED !== $property->default) { | |||
There was a problem hiding this comment.
This will only work for properties where the default has been set via an annotation or attribute. Not for assigned defaults. Or am I missing something?
There was a problem hiding this comment.
that's correct. I am attempting to look for a better solution
There was a problem hiding this comment.
Feel free to use the tests that I have already created in #2186
There was a problem hiding this comment.
From my tests so far it looks like the default value from the property assignement is nowhere actually added to the properties schema...
Should that not be happening?
It didn't seem to happen in 4.14.0 either, so the change towards 4.15.0 – marking things that are not nullable as required – seems to be perfectly valid. The properties should never have been marked as nullable in the first place as they do contain a default value and are therefore not nullable even when no parameter is passed in. The default parameter seems to just never have been recognized - unless separately declared in the Property-definition, which should not be necessary when it is retrievable from the code.
There was a problem hiding this comment.
That is also what I noticed, I have a local branch experimenting with a ReflectionPropertyDescriber which allows us to actually check if a default is assigned (and set it in the schema if not yet manually defined) to a property.
This should also prevent any unwanted required from being set.
I am waiting for #2184 to actually make a somewhat better setup for this new describer.
There was a problem hiding this comment.
Would it be possible to separate those two things from one another? So that it would be possible to no longer have the attributes with defaults assigned marked as required?
As that is kinda a bug that is probably currently breaking some API-Docs already I fix for that would be great.
In a project where I encountered the issue I have currently set the requirement hard to 4.14.0 as that keeps the expected behaviour, but that should probably not be the default.
There was a problem hiding this comment.
It think it makes sense to revert #2141 for now and wait for a proper solution for documenting the required property in schemas.
There was a problem hiding this comment.
You might want to have a look at the previously mentioned PR. I think I found a not too hacky solution.
At least my new tests as well as previous tests run - though with slight modifications where the previous expectations were broken in the light of the PRs scope
fixes #2179