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.

Sign up for GitHub

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

fix: set nullable true when default value is null #2390

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix: set nullable true when default value is null #2390

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 60 additions & 11 deletions src/ModelDescriber/Annotations/ReflectionReader.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,16 +54,26 @@
if ($reflection instanceof \ReflectionMethod) {
$methodDefault = $this->getDefaultFromMethodReflection($reflection);
if (Generator::UNDEFINED !== $methodDefault) {
if (null === $methodDefault) {
$property->nullable = true;

return;
}
$property->default = $methodDefault;

return;
}
}

if ($reflection instanceof \ReflectionProperty) {
$methodDefault = $this->getDefaultFromPropertyReflection($reflection);
if (Generator::UNDEFINED !== $methodDefault) {
$property->default = $methodDefault;
$propertyDefault = $this->getDefaultFromPropertyReflection($reflection);
if (Generator::UNDEFINED !== $propertyDefault) {
if (Generator::UNDEFINED === $property->nullable && null === $propertyDefault) {
$property->nullable = true;

return;
}
$property->default = $propertyDefault;

return;
}
Expand All @@ -77,6 +87,7 @@
if ($parameter->name !== $serializedName) {
continue;
}

if (!$parameter->isDefaultValueAvailable()) {
continue;
}
Expand All @@ -89,7 +100,12 @@
continue;
}

$property->default = $parameter->getDefaultValue();
$default = $parameter->getDefaultValue();
if (Generator::UNDEFINED === $property->nullable && null === $default) {
$property->nullable = true;
}

$property->default = $default;
}
}

Expand Down Expand Up @@ -117,10 +133,6 @@
return Generator::UNDEFINED;
}

if (null === $param->getDefaultValue()) {
return Generator::UNDEFINED;
}

return $param->getDefaultValue();
}

Expand All @@ -134,12 +146,49 @@
return Generator::UNDEFINED;
}

$defaultValue = $reflection->getDeclaringClass()->getDefaultProperties()[$propertyName] ?? null;
if (PHP_VERSION_ID < 80000) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does this need to check the PHP version? I would rather this not be the case because the next major 5.x will move to PHP 8.1 minimum

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As hasDefaultValue and getDefaultValue are only available since PHP8.0 I needed this "workaround" for the PHP7 users that uses a quirky way of getting the default values. That way also does not take into account possible DocBlock types so it might report back an unexpected default value when an implicit null is set. In PHP8.0 and higher we check for DocBlock type-infos to make a possible NULL default value plausible or not.

In he next major release this part can safely be removed as PHP8.1 supports the required functions 😉

return $reflection->getDeclaringClass()->getDefaultProperties()[$propertyName] ?? Generator::UNDEFINED;

Check warning on line 150 in src/ModelDescriber/Annotations/ReflectionReader.php

View check run for this annotation

Codecov / codecov/patch

src/ModelDescriber/Annotations/ReflectionReader.php#L150 

Added line #L150 was not covered by tests
}

if (!$reflection->hasDefaultValue()) {
return Generator::UNDEFINED;
}

if (null === $defaultValue) {
if ($this->hasImplicitNullDefaultValue($reflection)) {
return Generator::UNDEFINED;
}

return $defaultValue;
return $reflection->getDefaultValue();
}

/**
* Check whether the default value is an implicit null.
*
* An implicit null would be any null value that is not explicitly set and
* contradicts a set DocBlock @ var annotation
*
* So a property without an explicit value but an `@var int` docblock
* would be considered as not having an implicit null default value as
* that contradicts the annotation.
*
* A property without a default value and no docblock would be considered
* to have an explicit NULL default value to be set though.
*/
private function hasImplicitNullDefaultValue(\ReflectionProperty $reflection): bool
{
if (null !== $reflection->getDefaultValue()) {
return false;
}

if (false === $reflection->getDocComment()) {
return true;
}

$docComment = $reflection->getDocComment();
if (!preg_match('/@var\s+([^\s]+)/', $docComment, $matches)) {
return true;
}

return false === strpos(strtolower($matches[1]), 'null');
}
}
Loading