[3.x] Added parameter and return types to the PersistentCollectionTrait #2939
Conversation
krzysztof-ciszewski
changed the title
IonBazan
left a comment
IonBazan
left a comment
There was a problem hiding this comment.
Please also check remaining methods in that file and add the types there
| * @return true | ||
| */ | ||
| private function doAdd($value, $arrayAccess) | ||
| private function doAdd($value, bool $arrayAccess): true |
| } | ||
|
|
||
| public function add($element) | ||
| public function add($element): bool |
There was a problem hiding this comment.
| public function add($element): bool | |
| public function add(mixed $element): bool |
| } | ||
|
|
||
| public function set($key, $value) | ||
| public function set($key, $value): void |
There was a problem hiding this comment.
Let's see if we can add types to the key and value here too
| /** @return void */ | ||
| #[ReturnTypeWillChange] | ||
| public function offsetUnset($offset) | ||
| public function offsetUnset(mixed $offset) |
There was a problem hiding this comment.
| public function offsetUnset(mixed $offset) | |
| public function offsetUnset(mixed $offset): void |
This also allows to remove the PHP doc and #[ReturnTypeWillChange]
| } | ||
|
|
||
| /** | ||
| * @phpstan-return (TMaybeContained is T ? TKey|false : false) |
There was a problem hiding this comment.
Pull request overview
This PR adds parameter and return types to the PersistentCollectionTrait in preparation for a doctrine/collections update. This is marked as a breaking change, as it introduces strict type declarations throughout the trait and corresponding interface.
Key changes:
- Added return types to all public methods in
PersistentCollectionTraitandPersistentCollectionInterface - Added parameter types where missing (e.g., converting
$dirtytobool $dirty) - Updated PHPDoc annotations to align with new type signatures
- Updated test mocks to match new return type expectations
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
tests/Tests/PersistentCollectionTest.php |
Updated test mocks to return correct types matching new method signatures (empty array for slice(), null for remove()) |
src/PersistentCollection/PersistentCollectionTrait.php |
Added comprehensive return types and parameter types to all trait methods, with supporting PHPDoc annotations |
src/PersistentCollection/PersistentCollectionInterface.php |
Added return types to interface methods and cleaned up redundant PHPDoc annotations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public function getMapping() | ||
| /** @return array<string, mixed> */ | ||
| public function getMapping(): array | ||
| { |
There was a problem hiding this comment.
The getMapping() method declares a return type of array, but the $mapping property is declared as ?array (nullable) on line 57 and initialized to null. This method can return null, which violates the type declaration. The return type should be array|null or the method should throw an exception when $mapping is null.
| { | |
| { | |
| if ($this->mapping === null) { | |
| throw new MongoDBException('No mapping is associated with this PersistentCollection, please set one using setOwner method.'); | |
| } |
| } | ||
|
|
||
| public function set($key, $value) | ||
| public function set($key, $value): void |
There was a problem hiding this comment.
The set() method is missing parameter types. Based on the PHPDoc annotations for similar methods and the template parameters, $key should be typed as TKey (string|int) and $value should be typed as T (mixed or object). Consider adding type declarations for consistency with other methods in this trait.
| public function set($key, $value): void | |
| public function set(string|int $key, mixed $value): void |
| } | ||
|
|
||
| public function add($element) | ||
| public function add($element): bool |
There was a problem hiding this comment.
The add() method is missing a parameter type for $element. Based on the template parameter T of object and consistency with other methods, this should be typed. Consider adding a type declaration for the parameter.
| public function add($element): bool | |
| public function add(object $element): bool |
| } | ||
|
|
||
| public function removeElement($element) | ||
| public function removeElement($element): bool |
There was a problem hiding this comment.
The removeElement() method is missing a parameter type for $element. Based on the template parameter T of object and consistency with other methods in this trait, this parameter should have a type declaration.
| public function removeElement($element): bool | |
| public function removeElement(object $element): bool |
2 participants
Summary
Added parameter and return types to the PersistentCollectionTrait in preparation for doctrine/collection update