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

Fix more psalm findings #2338

Merged
merged 6 commits into from
Jul 18, 2021
Merged

Fix more psalm findings #2338

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3e9d22a
Fix phpdoc types
franmomu Jul 6, 2021
783744b
Force string as second parameter
franmomu Jul 6, 2021
17b4d76
Change to return void
franmomu Jul 6, 2021
9f9c8ee
Ignore psalm returning generic type issues
franmomu Jul 6, 2021
e9cdeb3
Use strpos instead of offset
franmomu Jul 6, 2021
4213692
Force key as string
franmomu Jul 18, 2021
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: 3 additions & 4 deletions lib/Doctrine/ODM/MongoDB/DocumentManager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -306,13 +306,12 @@ public function getClassNameResolver(): ClassNameResolver
* @psalm-return ClassMetadata<T>
*
* @template T of object
*
* @psalm-suppress InvalidReturnType, InvalidReturnStatement see https://github.com/vimeo/psalm/issues/5788
*/
public function getClassMetadata($className): ClassMetadata
{
$metadata = $this->metadataFactory->getMetadataFor($className);
assert($metadata instanceof ClassMetadata);
Copy link
Contributor Author

@franmomu franmomu Jul 6, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I added this assert in another PR to make SA tools happy, but since this is a performance sensitive method, I think it's better to just ignore the issue which is related to vimeo/psalm#5788

Copy link
Member

Choose a reason for hiding this comment

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

People are meant to disable assert in production mode anyway :)

Copy link
Member

Choose a reason for hiding this comment

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

But given we need @psalm-suppress for two other things I'm fine with this 👍


return $metadata;
return $this->metadataFactory->getMetadataFor($className);
}

/**
Expand Down
2 changes: 1 addition & 1 deletion lib/Doctrine/ODM/MongoDB/Mapping/Driver/XmlDriver.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -414,7 +414,7 @@ private function addEmbedMapping(ClassMetadata $class, SimpleXMLElement $embed,
}

if (isset($attributes['also-load'])) {
$mapping['alsoLoadFields'] = explode(',', $attributes['also-load']);
$mapping['alsoLoadFields'] = explode(',', (string) $attributes['also-load']);
}

$this->addFieldMapping($class, $mapping);
Expand Down
2 changes: 1 addition & 1 deletion lib/Doctrine/ODM/MongoDB/Proxy/Factory/StaticProxyFactory.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ private function createInitializer(
}

/**
* @return string[]
* @return array<int, 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.

Change to be in sync with $proxyOptions of https://github.com/Ocramius/ProxyManager/blob/21e2b4aa7d7661e7641cc6362fc8635ddcfa8464/src/ProxyManager/Factory/LazyLoadingGhostFactory.php#L85

*/
private function skippedFieldsFqns(ClassMetadata $metadata): array
{
Expand Down
6 changes: 3 additions & 3 deletions lib/Doctrine/ODM/MongoDB/SchemaManager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -742,7 +742,7 @@ public function enableShardingForDbByDocumentName(string $documentName): void
}
}

private function runShardCollectionCommand(string $documentName, ?WriteConcern $writeConcern = null): array
private function runShardCollectionCommand(string $documentName, ?WriteConcern $writeConcern = null): void
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 value was used prior to #1848, but after that it is not used anymore.

{
$class = $this->dm->getClassMetadata($documentName);
$dbName = $this->dm->getDocumentDatabase($documentName)->getDatabaseName();
Expand All @@ -765,15 +765,15 @@ private function runShardCollectionCommand(string $documentName, ?WriteConcern $
$shardKeyPart[$fieldName] = $order;
}

return $adminDb->command(
$adminDb->command(
array_merge(
[
'shardCollection' => $dbName . '.' . $class->getCollection(),
'key' => $shardKeyPart,
],
$this->getWriteOptions(null, $writeConcern)
)
)->toArray()[0];
);
}

private function ensureGridFSIndexes(ClassMetadata $class, ?int $maxTimeMs = null, ?WriteConcern $writeConcern = null, bool $background = false): void
Expand Down
6 changes: 5 additions & 1 deletion lib/Doctrine/ODM/MongoDB/UnitOfWork.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,7 +213,7 @@ final class UnitOfWork implements PropertyChangedListener
* At the end of the UnitOfWork all these collections will make new snapshots
* of their data.
*
* @psalm-var array<string, PersistentCollectionInterface<array-key, object>>
* @psalm-var array<string, array<PersistentCollectionInterface<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.

my fault, it was an array containing another one:

mongodb-odm/lib/Doctrine/ODM/MongoDB/UnitOfWork.php

Line 981 in 37ac643

$this->visitedCollections[spl_object_hash($topmostOwner)][] = $value;

*/
private $visitedCollections = [];

Expand Down Expand Up @@ -1626,6 +1626,8 @@ public function removeFromIdentityMap(object $document): bool
* @throws InvalidArgumentException If the class does not have an identifier.
*
* @template T of object
*
* @psalm-suppress InvalidReturnStatement, InvalidReturnType because of the inability of defining a generic property map
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 two suppresses are because the same cause than in #2328 (comment)

*/
public function getById($id, ClassMetadata $class): object
{
Expand Down Expand Up @@ -1653,6 +1655,8 @@ public function getById($id, ClassMetadata $class): object
* @throws InvalidArgumentException If the class does not have an identifier.
*
* @template T of object
*
* @psalm-suppress InvalidReturnStatement, InvalidReturnType because of the inability of defining a generic property map
*/
public function tryGetById($id, ClassMetadata $class)
{
Expand Down