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

Make PersistentCollectionInterface and PersistentCollection generic #2324

Merged
merged 2 commits into from
Jun 24, 2021

Conversation

franmomu
Copy link
Contributor

@franmomu franmomu commented Jun 6, 2021

Q A
Type improvement
BC Break no
Fixed issues

Summary

Adding more generics, in this case to PersistentCollectionInterface and PersistentCollection. While adding these changes, other problems arose, I'll add comments to the code so it's easier to review.

@@ -99,7 +100,6 @@
* isOwningSide: bool,
* isInverseSide: bool,
* strategy?: string,
* notSaved?: bool,
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 is duplicated

@@ -85,7 +85,8 @@
* version?: bool,
* lock?: bool,
* inherited?: string,
* declared?: class-string
* declared?: class-string,
* prime?: list<string>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Missing one, I used list<string> because of:

foreach ($reference->{'prime'}->{'field'} as $field) {
$attr = $field->attributes();
$mapping['prime'][] = (string) $attr['name'];
}

*
* @return bool
* @return bool|T|null
Copy link
Contributor Author

@franmomu franmomu Jun 6, 2021

Choose a reason for hiding this comment

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

Here we can return the result of $removed = $this->coll->remove($offset); which is T|null

@@ -268,3 +268,33 @@ parameters:
message: "#^Call to method PHPUnit\\\\Framework\\\\Assert\\:\\:assertInstanceOf\\(\\) with 'DateTime' and null will always evaluate to false\\.$#"
count: 1
path: tests/Doctrine/ODM/MongoDB/Tests/Types/DateTypeTest.php

# import type in PHPStan does not work, see https://github.com/phpstan/phpstan/issues/5091
Copy link
Contributor Author

@franmomu franmomu Jun 6, 2021

Choose a reason for hiding this comment

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

All these issues being ignored are because in PHPStan, apparently importing type aliases from a trait does not work: phpstan/phpstan#5091

Copy link
Member

Choose a reason for hiding this comment

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

Subscribed to that issue 👍

Comment on lines +45 to +57
$unserialized->setOwner($owner, [
'type' => ClassMetadata::ONE,
'name' => 'name',
'fieldName' => 'fieldName',
'isCascadeRemove' => false,
'isCascadePersist' => false,
'isCascadeRefresh' => false,
'isCascadeMerge' => false,
'isCascadeDetach' => false,
'isOwningSide' => false,
'isInverseSide' => false,
'targetDocument' => stdClass::class,
]);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is to create a valid FieldMapping.

$pcoll = new PersistentCollection($collection, $this->dm, $this->uow);
$this->assertSame(2, $pcoll[0]);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These changes in tests are because PersistentCollection is not supposed to be handling scalars.

*/
private $snapshot = [];

/**
* Collection's owning entity
*
* @var object|null
* @psalm-var T|null
Copy link
Member

Choose a reason for hiding this comment

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

This one seems off: the owner can be (and usual is) of other type than what's inside of the collection

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, you are right, changing it.

@@ -268,3 +268,33 @@ parameters:
message: "#^Call to method PHPUnit\\\\Framework\\\\Assert\\:\\:assertInstanceOf\\(\\) with 'DateTime' and null will always evaluate to false\\.$#"
count: 1
path: tests/Doctrine/ODM/MongoDB/Tests/Types/DateTypeTest.php

# import type in PHPStan does not work, see https://github.com/phpstan/phpstan/issues/5091
Copy link
Member

Choose a reason for hiding this comment

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

Subscribed to that issue 👍

@malarzm malarzm added the Task label Jun 22, 2021
@malarzm malarzm added this to the 2.3.0 milestone Jun 22, 2021
@franmomu franmomu force-pushed the make_persist_collection_generic branch from eec7e12 to d8523ac Compare June 24, 2021 06:58
@franmomu franmomu force-pushed the make_persist_collection_generic branch from d8523ac to 5e57d0e Compare June 24, 2021 07:01
@malarzm malarzm merged commit 1db4828 into doctrine:2.3.x Jun 24, 2021
@malarzm
Copy link
Member

malarzm commented Jun 24, 2021

Thanks @franmomu!

@franmomu franmomu deleted the make_persist_collection_generic branch June 24, 2021 07:10
@alcaeus alcaeus modified the milestones: 2.3.0-alpha1, 2.3.0 Sep 9, 2021
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.

3 participants