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

Jump to bottom

Solve some PHPStan baseline errors #10242

Merged
merged 2 commits into from
Nov 21, 2022
Merged

Solve some PHPStan baseline errors #10242

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
47b4ccc
Solve some baseline errors
VincentLanglet Nov 20, 2022
db7333c
Update baseline
VincentLanglet Nov 20, 2022
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
7 changes: 4 additions & 3 deletions lib/Doctrine/ORM/PersistentCollection.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ final class PersistentCollection extends AbstractLazyCollection implements Selec
*
* @param EntityManagerInterface $em The EntityManager the collection will be associated with.
* @param ClassMetadata $class The class descriptor of the entity type of this collection.
* @psalm-param Collection<TKey, T> $collection The collection elements.
* @psalm-param Collection<TKey, T>&Selectable<TKey, T> $collection The collection elements.
*/
public function __construct(EntityManagerInterface $em, $class, Collection $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.

Since unwrap need to return a Selectable (cf next comment), we need to only accepts selectable.

{
Expand Down Expand Up @@ -675,11 +675,12 @@ public function matching(Criteria $criteria): Collection
/**
* Retrieves the wrapped Collection instance.
*
* @return Collection<TKey, T>
* @return Collection<TKey, T>&Selectable<TKey, T>
*/
public function unwrap(): 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.

In the matching method, there is the code

public function matching(Criteria $criteria): Collection
    {
        if ($this->isDirty) {
            $this->initialize();
        }

        if ($this->initialized) {
            return $this->unwrap()->matching($criteria);
        }

So, unwrap need to return a Selectable if we want to call matching on it.

{
assert($this->collection !== null);
assert($this->collection instanceof Collection);
assert($this->collection instanceof Selectable);

return $this->collection;
}
Expand Down
16 changes: 5 additions & 11 deletions lib/Doctrine/ORM/UnitOfWork.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,14 +222,14 @@ class UnitOfWork implements PropertyChangedListener
/**
* All pending collection deletions.
*
* @psalm-var array<int, Collection<array-key, object>>
* @psalm-var array<int, PersistentCollection<array-key, object>>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We only add PersistentCollection to this array

if ($orgValue instanceof PersistentCollection) {
                    // A PersistentCollection was de-referenced, so delete it.
                    $coid = spl_object_id($orgValue);

                    if (isset($this->collectionDeletions[$coid])) {
                        continue;
                    }

                    $this->collectionDeletions[$coid] = $orgValue;
                    $changeSet[$propName]             = $orgValue; // Signal changeset, to-many assocs will be ignored.

                    continue;
                }

*/
private $collectionDeletions = [];

/**
* All pending collection updates.
*
* @psalm-var array<int, Collection<array-key, object>>
* @psalm-var array<int, PersistentCollection<array-key, object>>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We only add PersistentCollection to this array

if ($value instanceof PersistentCollection && $value->isDirty()) {
            $coid = spl_object_id($value);

            $this->collectionUpdates[$coid]  = $value;
            $this->visitedCollections[$coid] = $value;
        }

*/
private $collectionUpdates = [];

Expand All @@ -238,7 +238,7 @@ class UnitOfWork implements PropertyChangedListener
* At the end of the UnitOfWork all these collections will make new snapshots
* of their data.
*
* @psalm-var array<int, Collection<array-key, object>>
* @psalm-var array<int, PersistentCollection<array-key, object>>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We only add PersistentCollection to this array

if ($value instanceof PersistentCollection && $value->isDirty()) {
            $coid = spl_object_id($value);

            $this->collectionUpdates[$coid]  = $value;
            $this->visitedCollections[$coid] = $value;
        }

*/
private $visitedCollections = [];

Expand Down Expand Up @@ -419,12 +419,6 @@ public function commit($entity = null)
try {
// Collection deletions (deletions of complete collections)
foreach ($this->collectionDeletions as $collectionToDelete) {
if (! $collectionToDelete instanceof PersistentCollection) {
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 not possible since collectionDeletions is always containing PersistentCollection.

  • when looking at the added value they are always persistent collection
  • it it was possible to go in this if, we would have called $collectionToDelete->getMapping() but getMapping only exists for PersistentCollection.

Copy link
Member

Choose a reason for hiding this comment

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

This code was deliberately added in #7861, which is a bit worrying, but also, the test that was added in that PR still passes, so I guess it's fine?

Copy link
Contributor Author

@VincentLanglet VincentLanglet Nov 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Yes. I would say that the developer wanted to be BC

  • If not PersistentCollection do like before
  • If PersistentCollection add the bugfix

But it's not possible to have something else than PersistentCollection ^^
(You can look at the place where collectionDeletions is updated)

$this->getCollectionPersister($collectionToDelete->getMapping())->delete($collectionToDelete);

continue;
}

// Deferred explicit tracked collections can be removed only when owning relation was persisted
$owner = $collectionToDelete->getOwner();

Expand Down Expand Up @@ -3376,7 +3370,7 @@ public function getScheduledEntityDeletions()
/**
* Gets the currently scheduled complete collection deletions
*
* @psalm-return array<int, Collection<array-key, object>>
* @psalm-return array<int, PersistentCollection<array-key, object>>
*/
public function getScheduledCollectionDeletions()
{
Expand All @@ -3386,7 +3380,7 @@ public function getScheduledCollectionDeletions()
/**
* Gets the currently scheduled collection inserts, updates and deletes.
*
* @psalm-return array<int, Collection<array-key, object>>
* @psalm-return array<int, PersistentCollection<array-key, object>>
*/
public function getScheduledCollectionUpdates()
{
Expand Down
24 changes: 2 additions & 22 deletions phpstan-baseline.neon
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,11 +250,6 @@ parameters:
count: 1
path: lib/Doctrine/ORM/NativeQuery.php

-
message: "#^Call to an undefined method Doctrine\\\\Common\\\\Collections\\\\Collection\\<TKey of \\(int\\|string\\), T\\>\\:\\:matching\\(\\)\\.$#"
count: 1
path: lib/Doctrine/ORM/PersistentCollection.php

-
message: "#^Method Doctrine\\\\ORM\\\\Persisters\\\\Collection\\\\OneToManyPersister\\:\\:delete\\(\\) should return int\\|null but empty return statement found\\.$#"
count: 1
Expand Down Expand Up @@ -716,27 +711,12 @@ parameters:
path: lib/Doctrine/ORM/Tools/SchemaTool.php

-
message: "#^Call to an undefined method Doctrine\\\\Common\\\\Collections\\\\Collection\\<\\(int\\|string\\), object\\>\\:\\:getMapping\\(\\)\\.$#"
count: 2
path: lib/Doctrine/ORM/UnitOfWork.php

-
message: "#^Call to an undefined method Doctrine\\\\Common\\\\Collections\\\\Collection\\<\\(int\\|string\\), object\\>\\:\\:takeSnapshot\\(\\)\\.$#"
count: 1
path: lib/Doctrine/ORM/UnitOfWork.php

-
message: "#^Parameter \\#1 \\$collection of method Doctrine\\\\ORM\\\\Persisters\\\\Collection\\\\CollectionPersister\\:\\:delete\\(\\) expects Doctrine\\\\ORM\\\\PersistentCollection, Doctrine\\\\Common\\\\Collections\\\\Collection\\<\\(int\\|string\\), object\\> given\\.$#"
count: 1
path: lib/Doctrine/ORM/UnitOfWork.php

-
message: "#^Parameter \\#1 \\$collection of method Doctrine\\\\ORM\\\\Persisters\\\\Collection\\\\CollectionPersister\\:\\:update\\(\\) expects Doctrine\\\\ORM\\\\PersistentCollection, Doctrine\\\\Common\\\\Collections\\\\Collection\\<\\(int\\|string\\), object\\> given\\.$#"
message: "#^Parameter \\#3 \\$changeSet of class Doctrine\\\\ORM\\\\Event\\\\PreUpdateEventArgs constructor is passed by reference, so it expects variables only$#"
count: 1
path: lib/Doctrine/ORM/UnitOfWork.php

-
message: "#^Parameter \\#3 \\$changeSet of class Doctrine\\\\ORM\\\\Event\\\\PreUpdateEventArgs constructor is passed by reference, so it expects variables only$#"
message: "#^Parameter \\#3 \\$collection of class Doctrine\\\\ORM\\\\PersistentCollection constructor expects Doctrine\\\\Common\\\\Collections\\\\Collection\\<\\(int\\|string\\), mixed\\>&Doctrine\\\\Common\\\\Collections\\\\Selectable\\<\\(int\\|string\\), mixed\\>, Doctrine\\\\Common\\\\Collections\\\\Collection given\\.$#"
count: 1
path: lib/Doctrine/ORM/UnitOfWork.php

Expand Down
29 changes: 17 additions & 12 deletions psalm-baseline.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1031,6 +1031,11 @@
<code>$entity</code>
</PossiblyInvalidArgument>
</file>
<file src="lib/Doctrine/ORM/ORMSetup.php">
<UndefinedClass occurrences="1">
<code>MemcachedAdapter::createConnection('memcached://127.0.0.1')</code>
</UndefinedClass>
</file>
<file src="lib/Doctrine/ORM/PersistentCollection.php">
<ImplementedReturnTypeMismatch occurrences="2">
<code>Collection&lt;TKey, T&gt;</code>
Expand All @@ -1039,6 +1044,12 @@
<InvalidReturnStatement occurrences="2">
<code>$this-&gt;em-&gt;find($this-&gt;typeClass-&gt;name, $key)</code>
</InvalidReturnStatement>
<InvalidReturnType occurrences="1">
<code>Collection&lt;TKey, T&gt;</code>
</InvalidReturnType>
<LessSpecificReturnStatement occurrences="1">
<code>$this-&gt;unwrap()-&gt;matching($criteria)</code>
</LessSpecificReturnStatement>
<MissingParamType occurrences="1">
<code>$offset</code>
</MissingParamType>
Expand Down Expand Up @@ -1070,9 +1081,6 @@
<code>setValue</code>
<code>setValue</code>
</PossiblyNullReference>
<UndefinedInterfaceMethod occurrences="1">
<code>matching</code>
</UndefinedInterfaceMethod>
</file>
<file src="lib/Doctrine/ORM/Persisters/Collection/ManyToManyPersister.php">
<PossiblyNullArgument occurrences="44">
Expand Down Expand Up @@ -2800,11 +2808,9 @@
</UnresolvableInclude>
</file>
<file src="lib/Doctrine/ORM/UnitOfWork.php">
<ArgumentTypeCoercion occurrences="5">
<ArgumentTypeCoercion occurrences="3">
<code>$class</code>
<code>$class</code>
<code>$collectionToDelete</code>
<code>$collectionToUpdate</code>
<code>$commitOrder[$i]</code>
</ArgumentTypeCoercion>
<DocblockTypeContradiction occurrences="2">
Expand All @@ -2829,10 +2835,13 @@
<NullableReturnStatement occurrences="1">
<code>$this-&gt;identityMap[$rootClassName][$idHash]</code>
</NullableReturnStatement>
<PossiblyInvalidArgument occurrences="1">
<code>$value</code>
</PossiblyInvalidArgument>
<PossiblyInvalidArrayOffset occurrences="1">
<code>$this-&gt;identityMap[$rootClassName]</code>
</PossiblyInvalidArrayOffset>
<PossiblyNullArgument occurrences="12">
<PossiblyNullArgument occurrences="13">
<code>$assoc</code>
<code>$assoc</code>
<code>$assoc</code>
Expand All @@ -2842,6 +2851,7 @@
<code>$collection-&gt;getOwner()</code>
<code>$collection-&gt;getOwner()</code>
<code>$collectionToDelete-&gt;getMapping()</code>
<code>$collectionToUpdate-&gt;getMapping()</code>
<code>$entity</code>
<code>$entity</code>
<code>$owner</code>
Expand Down Expand Up @@ -2905,11 +2915,6 @@
<ReferenceConstraintViolation occurrences="1">
<code>$visited</code>
</ReferenceConstraintViolation>
<UndefinedInterfaceMethod occurrences="3">
<code>getMapping</code>
<code>getMapping</code>
<code>takeSnapshot</code>
</UndefinedInterfaceMethod>
</file>
<file src="lib/Doctrine/ORM/Utility/HierarchyDiscriminatorResolver.php">
<NoInterfaceProperties occurrences="2">
Expand Down