Closed victorstanciu closed 2 years ago
- Is there a reason why
hasProperty()
also checks if an attribute is defined? The method kinda feels misnamed.
yeah, no idea why this method even exists, it is only called in that one place, so I'm going to fix it this way.
actually came up with an even better fix, added a new method and deprecated hasProperty()
so this does not break any extension that might use it.
The failing tests from PR #132 were merged into master, so now all the checks are failing for new PRs:
The issue is caused by this ternary operator in
src/spec/Schema.php
, which will always evaluate totrue
(thereby always settingnullable
tofalse
, and never actuallynull
):https://github.com/cebe/php-openapi/blob/336bd8d3ce449413cbb75273c17a00f13a16426c/src/spec/Schema.php#L130
This always evaluates to the
true
side of the operator becausehasProperty()
also returnstrue
if an attribute is defined, whether or not it has a value:https://github.com/cebe/php-openapi/blob/336bd8d3ce449413cbb75273c17a00f13a16426c/src/SpecBaseObject.php#L300-L303
And of course,
type
is a defined attribute of theSchema
object:https://github.com/cebe/php-openapi/blob/336bd8d3ce449413cbb75273c17a00f13a16426c/src/spec/Schema.php#L92
I see two quick solutions to this problem, but I wanted to discuss them with you guys before I open a PR:
As far as I see,
Schema::attributeDefaults()
is the only place where theSpecBaseObject::hasProperty()
method is called, so a simple fix (which passes all the tests) would be to simplify the method like this:Is there a reason why
hasProperty()
also checks if an attribute is defined? The method kinda feels misnamed.The ternary conditional in
attributeDefaults()
could be rewritten to useisset
:But
isset
of course calls__isset()
, which itself callsattributeDefaults()
, so this might cause a recursion error.Thoughts?