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

Use PHPStan 1 #2385

Merged
merged 2 commits into from
Nov 18, 2021
Merged

Use PHPStan 1 #2385

merged 2 commits into from
Nov 18, 2021

Conversation

franmomu
Copy link
Contributor

@franmomu franmomu commented Nov 2, 2021

Q A
Type improvement
BC Break no
Fixed issues

Summary

It's made on top of #2383, once that is merge I'll rebase this one

@franmomu franmomu added the Task label Nov 2, 2021
@@ -1356,7 +1363,7 @@ public function getCollection(): string
* Sets the collection this Document is mapped to.
*
* @param array|string $name
* @psalm-param array{name: string, capped?: bool, size?: int, max?: int}|string $name
* @psalm-param array{name?: string, capped?: bool, size?: int, max?: int}|string $name
Copy link
Member

Choose a reason for hiding this comment

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

Why this? name is actually required to be present in the array :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was because of the isset check below, I've added treatPhpDocTypesAsCertain option as false which seems cleaner.

@@ -243,6 +243,7 @@ public function matching(Criteria $criteria): ArrayCollection
}

// @TODO: wrap around a specialized Collection for efficient count on large collections
/** @var Iterator<int, T> $iterator */
$iterator = $queryBuilder->getQuery()->execute();
assert($iterator instanceof Iterator);
Copy link
Member

Choose a reason for hiding this comment

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

I guess the assert can be removed now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah we can, I left it because if we reach level 6 of PHPStan or achieve to make query builder generic, we could get rid of the @var annotation, but we can remove it in the meanwhile.

Copy link
Member

Choose a reason for hiding this comment

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

No need in this case 👍

*/
protected $referencedChild;

/**
* @ODM\ReferenceMany(targetDocument=ChildDocumentWithoutDiscriminator::class)
*
* @var Collection<int, ChildDocumentWithDiscriminator>
* @var Collection<int, ChildDocumentWithDiscriminator>|array<ChildDocumentWithDiscriminator>
Copy link
Member

Choose a reason for hiding this comment

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

Personally I'm more for ChildDocumentWithDiscriminator[]. How do we tend to use these across the repository?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was using [] at first and then I changed them to this because I was getting (not in this case):

https://phpstan.org/r/3d620f33-b686-4cea-ad37-510b638b2845

Apparently that only happens when there is a default value of [] 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Weird, empty array should be good enough for anything array-ish :) Mind reporting it to PHPStan?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was looking at this and I found in https://phpstan.org/writing-php-code/phpdoc-types#iterables

These iterable rules are applied only when the Collection type isn’t generic. When it’s generic, generics rules for class-level type variables are applied.

If PHP encounters Collection|Foo[], two possible paths are taken:

Collection implements Traversable so Collection|Foo[] is interpreted as a Collection object that iterates over Foo. The array part isn’t applied.
Collection does not implement Traversable so Collection|Foo[] is interpreted as a Collection object or an array of Foo objects.
If Collection|Foo[] means "Collection or array" in your case even if Collection implements Traversable, you need to disambiguate the type by using Collection|array<Foo> instead.

So this last paragraph explains this case, good to know.

@franmomu franmomu added this to the 2.3.0 milestone Nov 17, 2021
@malarzm malarzm merged commit 240fc93 into doctrine:2.3.x Nov 18, 2021
@malarzm
Copy link
Member

malarzm commented Nov 18, 2021

Thanks @franmomu!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants