Unit tests are broken in master since PR #132

[victorstanciu]

The failing tests from PR #132 were merged into master, so now all the checks are failing for new PRs:

There was 1 failure:

1) SchemaTest::testNullable
Failed asserting that false is null.

/home/runner/work/php-openapi/php-openapi/tests/spec/SchemaTest.php:66

The issue is caused by this ternary operator in src/spec/Schema.php, which will always evaluate to true (thereby always setting nullable to false, and never actually null):

php-openapi/src/spec/Schema.php

Line 130 in 336bd8d

'nullable' => $this->hasProperty('type') ? false : null,

This always evaluates to the true side of the operator because hasProperty() also returns true if an attribute is defined, whether or not it has a value:

php-openapi/src/SpecBaseObject.php

Lines 300 to 303 in 336bd8d

	protected function hasProperty(string $name): bool
	{
	return isset($this->_properties[$name]) || isset($this->attributes()[$name]);
	}

And of course, type is a defined attribute of the Schema object:

php-openapi/src/spec/Schema.php

Line 92 in 336bd8d

'type' => Type::STRING,

I see two quick solutions to this problem, but I wanted to discuss them with you guys before I open a PR:

1. As far as I see, Schema::attributeDefaults() is the only place where the SpecBaseObject::hasProperty() method is called, so a simple fix (which passes all the tests) would be to simplify the method like this:

      protected function hasProperty(string $name): bool
      {
          return isset($this->_properties[$name]);
      }

   Is there a reason why hasProperty() also checks if an attribute is defined? The method kinda feels misnamed.

2. The ternary conditional in attributeDefaults() could be rewritten to use isset:

     'nullable' => isset($this->type) ? false : null,

   But isset of course calls __isset(), which itself calls attributeDefaults(), so this might cause a recursion error.

Thoughts?

[cebe]

1. 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.

[cebe]

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.