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.

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: respect ancestor property tags in model extensions #1037

Merged
merged 2 commits into from
Nov 19, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).

### Fixed
* No return type should be enforced for closure of tap helper
* Ensure the model extensions considers PHPDoc `@property` tags from ancestors, not just the model class itself

### Changed
* Improved return type for Collection::first, last, get, pull when giving a default value.
Expand Down
5 changes: 3 additions & 2 deletions src/Properties/ModelPropertyExtension.php
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
use ArrayObject;
use Illuminate\Database\Eloquent\Model;
use Illuminate\Support\Str;
use NunoMaduro\Larastan\Reflection\ReflectionHelper;
use PHPStan\PhpDoc\TypeStringResolver;
use PHPStan\Reflection\ClassReflection;
use PHPStan\Reflection\PropertiesClassReflectionExtension;
Expand All @@ -19,7 +20,7 @@
*/
final class ModelPropertyExtension implements PropertiesClassReflectionExtension
{
/** @var SchemaTable[] */
/** @var array<string, SchemaTable> */
private $tables = [];

/** @var TypeStringResolver */
Expand Down Expand Up @@ -51,7 +52,7 @@ public function hasProperty(ClassReflection $classReflection, string $propertyNa
return false;
}

if (array_key_exists($propertyName, $classReflection->getPropertyTags())) {
if (array_key_exists($propertyName, ReflectionHelper::collectPropertyTags($classReflection))) {
return false;
}

Expand Down
3 changes: 2 additions & 1 deletion src/Properties/ModelRelationsExtension.php
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
use Illuminate\Support\Str;
use NunoMaduro\Larastan\Concerns;
use NunoMaduro\Larastan\Methods\BuilderHelper;
use NunoMaduro\Larastan\Reflection\ReflectionHelper;
use NunoMaduro\Larastan\Types\RelationParserHelper;
use PHPStan\Analyser\OutOfClassScope;
use PHPStan\Reflection\ClassReflection;
Expand Down Expand Up @@ -49,7 +50,7 @@ public function hasProperty(ClassReflection $classReflection, string $propertyNa
return false;
}

if (array_key_exists($propertyName, $classReflection->getPropertyTags())) {
if (array_key_exists($propertyName, ReflectionHelper::collectPropertyTags($classReflection))) {
return false;
}

Expand Down
30 changes: 30 additions & 0 deletions src/Reflection/ReflectionHelper.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
<?php

declare(strict_types=1);

namespace NunoMaduro\Larastan\Reflection;

use PHPStan\PhpDoc\Tag\PropertyTag;
use PHPStan\Reflection\ClassReflection;

final class ReflectionHelper
{
/**
* Returns all property tags of the given class and its ancestors.
*
* In case of duplicates, the tags of the class itself have precedence.
* TODO consider the hierarchy to ensure correct precedence between ancestors
spawnia marked this conversation as resolved.
Show resolved Hide resolved
*
* @return array<string, PropertyTag>
*/
public static function collectPropertyTags(ClassReflection $classReflection): array
{
$allPropertyTags = $classReflection->getPropertyTags();

foreach ($classReflection->getAncestors() as $ancestor) {
$allPropertyTags += $ancestor->getPropertyTags();
}

return $allPropertyTags;
}
spawnia marked this conversation as resolved.
Show resolved Hide resolved
}
54 changes: 54 additions & 0 deletions tests/Features/Models/Relations.php
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
use Illuminate\Database\Eloquent\Model;
use Illuminate\Database\Eloquent\Relations\BelongsTo;
use Illuminate\Database\Eloquent\Relations\HasMany;
use Illuminate\Database\Eloquent\Relations\HasOne;
use Illuminate\Database\Eloquent\Relations\MorphTo;
use Illuminate\Database\Eloquent\Relations\MorphToMany;
use function PHPStan\Testing\assertType;
Expand Down Expand Up @@ -232,6 +233,26 @@ public function testBelongsToManyCreateReturnsCorrectModel(User $user): Post

return $user->posts()->create();
}

public function testNullableUser(ExtendsModelWithPropertyAnnotations $model): bool
{
return $model->nullableUser === null;
}

public function testNonNullableUser(ExtendsModelWithPropertyAnnotations $model): User
{
return $model->nonNullableUser;
}

public function testNullableFoo(ExtendsModelWithPropertyAnnotations $model): bool
{
return $model->nullableFoo === null;
}

public function testNonNullableFoo(ExtendsModelWithPropertyAnnotations $model): string
{
return $model->nonNullableFoo;
}
}

/**
Expand All @@ -258,6 +279,39 @@ public function relation(): HasMany
}
}

/**
* @property-read User|null $nullableUser
* @property-read User $nonNullableUser
* @property-read string|null $nullableFoo
* @property-read string $nonNullableFoo
*/
class ModelWithPropertyAnnotations extends Model
{
public function nullableUser(): HasOne
{
return $this->hasOne(User::class);
}

public function nonNullableUser(): HasOne
{
return $this->hasOne(User::class);
}

public function getNullableFooAttribute(): ?string
{
return rand() ? 'foo' : null;
}

public function getNonNullableFooAttribute(): string
{
return 'foo';
}
}

class ExtendsModelWithPropertyAnnotations extends ModelWithPropertyAnnotations
{
}

class Tag extends Model
{
/**
Expand Down