Skip to content

PHPLIB-599: Typing improvements #959

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

Merged
merged 10 commits into from
Sep 13, 2022
Merged

PHPLIB-599: Typing improvements #959

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
b0f110f
PHPLIB-600: Add type definitions for parameters
alcaeus Aug 24, 2022
d273370
PHPLIB-602: Add return type definitions where applicable
alcaeus Aug 24, 2022
68d68e3
Enable object type for parameters
alcaeus Aug 24, 2022
e6e08b3
Add comment about using declare
alcaeus Aug 25, 2022
012650f
Remove useless param and return annotations
alcaeus Aug 25, 2022
1dec7b2
Remove redundant type casts
alcaeus Aug 25, 2022
a32be43
Add return types to internal classes
alcaeus Aug 26, 2022
9a9742e
Use object consistently in GridFS classes
alcaeus Aug 26, 2022
c7272cc
Remove useless type annotations
alcaeus Aug 26, 2022
eb6fb73
Remove unnecessary cast
alcaeus Aug 26, 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
16 changes: 3 additions & 13 deletions phpcs.xml.dist
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,16 +105,13 @@
<!-- **************************************************************************** -->
<rule ref="SlevomatCodingStandard.TypeHints.ParameterTypeHint">
<properties>
<!-- Requires PHP 7.4 -->
<property name="enableObjectTypeHint" value="false" />
Copy link
Member Author

Choose a reason for hiding this comment

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

The comment was wrong - the object type was introduced in PHP 7.2.

<!-- Requires PHP 8.0 -->
<property name="enableMixedTypeHint" value="false" />
<!-- Requires PHP 8.0 -->
<property name="enableUnionTypeHint" value="false" />
</properties>

<exclude name="SlevomatCodingStandard.TypeHints.ParameterTypeHint.MissingTraversableTypeHintSpecification" />
<exclude name="SlevomatCodingStandard.TypeHints.ParameterTypeHint.UselessAnnotation" />
</rule>

<rule ref="SlevomatCodingStandard.TypeHints.PropertyTypeHint">
Expand All @@ -128,13 +125,10 @@
</properties>

<exclude name="SlevomatCodingStandard.TypeHints.PropertyTypeHint.MissingTraversableTypeHintSpecification" />
<exclude name="SlevomatCodingStandard.TypeHints.PropertyTypeHint.UselessAnnotation" />
</rule>

<rule ref="SlevomatCodingStandard.TypeHints.ReturnTypeHint">
<properties>
<!-- Requires PHP 7.2 -->
<property name="enableObjectTypeHint" value="false" />
<!-- Requires PHP 8.0 -->
<property name="enableStaticTypeHint" value="false" />
<!-- Requires PHP 8.0 -->
Expand All @@ -144,7 +138,6 @@
</properties>

<exclude name="SlevomatCodingStandard.TypeHints.ReturnTypeHint.MissingTraversableTypeHintSpecification" />
<exclude name="SlevomatCodingStandard.TypeHints.ReturnTypeHint.UselessAnnotation" />
</rule>


Expand All @@ -162,12 +155,9 @@
</rule>


<!-- *********************************************************************************** -->
<!-- Require native type hints for all parameters, properties, and return types in tests -->
<!-- *********************************************************************************** -->
<rule ref="SlevomatCodingStandard.TypeHints.ParameterTypeHint.MissingNativeTypeHint">
<exclude-pattern>src</exclude-pattern>
</rule>
<!-- *********************************************************** -->
<!-- Require native type hints for all code without a BC promise -->
<!-- *********************************************************** -->
<rule ref="SlevomatCodingStandard.TypeHints.PropertyTypeHint.MissingNativeTypeHint">
<exclude-pattern>src</exclude-pattern>
</rule>
Expand Down
10 changes: 3 additions & 7 deletions src/BulkWriteResult.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,16 +28,12 @@ class BulkWriteResult
/** @var WriteResult */
private $writeResult;

/** @var mixed[] */
/** @var array */
private $insertedIds;

/** @var boolean */
private $isAcknowledged;

/**
* @param WriteResult $writeResult
* @param mixed[] $insertedIds
*/
public function __construct(WriteResult $writeResult, array $insertedIds)
{
$this->writeResult = $writeResult;
Expand Down Expand Up @@ -90,7 +86,7 @@ public function getInsertedCount()
* field value. Any driver-generated ID will be a MongoDB\BSON\ObjectId
* instance.
*
* @return mixed[]
* @return array
*/
public function getInsertedIds()
{
Expand Down Expand Up @@ -165,7 +161,7 @@ public function getUpsertedCount()
* This method should only be called if the write was acknowledged.
*
* @see BulkWriteResult::isAcknowledged()
* @return mixed[]
* @return array
* @throws BadMethodCallException is the write result is unacknowledged
*/
public function getUpsertedIds()
Expand Down
15 changes: 4 additions & 11 deletions src/ChangeStream.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,8 +90,6 @@ class ChangeStream implements Iterator

/**
* @internal
* @param ChangeStreamIterator $iterator
* @param callable $resumeCallable
*/
public function __construct(ChangeStreamIterator $iterator, callable $resumeCallable)
{
Expand Down Expand Up @@ -194,10 +192,8 @@ public function valid()
* Determines if an exception is a resumable error.
*
* @see https://github.com/mongodb/specifications/blob/master/source/change-streams/change-streams.rst#resumable-error
* @param RuntimeException $exception
* @return boolean
*/
private function isResumableError(RuntimeException $exception)
private function isResumableError(RuntimeException $exception): bool
{
if ($exception instanceof ConnectionException) {
return true;
Expand All @@ -224,7 +220,7 @@ private function isResumableError(RuntimeException $exception)
* @param boolean $incrementKey Increment $key if there is a current result
* @throws ResumeTokenException
*/
private function onIteration($incrementKey)
private function onIteration(bool $incrementKey): void
{
/* If the cursorId is 0, the server has invalidated the cursor and we
* will never perform another getMore nor need to resume since any
Expand All @@ -251,10 +247,8 @@ private function onIteration($incrementKey)

/**
* Recreates the ChangeStreamIterator after a resumable server error.
*
* @return void
*/
private function resume()
private function resume(): void
{
$this->iterator = call_user_func($this->resumeCallable, $this->getResumeToken(), $this->hasAdvanced);
$this->iterator->rewind();
Expand All @@ -265,10 +259,9 @@ private function resume()
/**
* Either resumes after a resumable error or re-throws the exception.
*
* @param RuntimeException $exception
* @throws RuntimeException
*/
private function resumeOrThrow(RuntimeException $exception)
private function resumeOrThrow(RuntimeException $exception): void
{
if ($this->isResumableError($exception)) {
$this->resume();
Expand Down
13 changes: 6 additions & 7 deletions src/Client.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ class Client
* @throws DriverInvalidArgumentException for parameter/option parsing errors in the driver
* @throws DriverRuntimeException for other driver errors (e.g. connection errors)
*/
public function __construct($uri = 'mongodb://127.0.0.1/', array $uriOptions = [], array $driverOptions = [])
public function __construct(string $uri = 'mongodb://127.0.0.1/', array $uriOptions = [], array $driverOptions = [])
{
$driverOptions += ['typeMap' => self::$defaultTypeMap];

Expand All @@ -116,7 +116,7 @@ public function __construct($uri = 'mongodb://127.0.0.1/', array $uriOptions = [

$driverOptions['driver'] = $this->mergeDriverInfo($driverOptions['driver'] ?? []);

$this->uri = (string) $uri;
$this->uri = $uri;
$this->typeMap = $driverOptions['typeMap'] ?? null;

unset($driverOptions['typeMap']);
Expand Down Expand Up @@ -155,7 +155,7 @@ public function __debugInfo()
* @param string $databaseName Name of the database to select
* @return Database
*/
public function __get($databaseName)
public function __get(string $databaseName)
{
return $this->selectDatabase($databaseName);
}
Expand Down Expand Up @@ -201,7 +201,7 @@ public function createClientEncryption(array $options)
* @throws InvalidArgumentException for parameter/option parsing errors
* @throws DriverRuntimeException for other driver errors (e.g. connection errors)
*/
public function dropDatabase($databaseName, array $options = [])
public function dropDatabase(string $databaseName, array $options = [])
{
if (! isset($options['typeMap'])) {
$options['typeMap'] = $this->typeMap;
Expand Down Expand Up @@ -290,7 +290,6 @@ public function listDatabaseNames(array $options = []): Iterator
* List databases.
*
* @see ListDatabases::__construct() for supported options
* @param array $options
* @return DatabaseInfoIterator
* @throws UnexpectedValueException if the command response was malformed
* @throws InvalidArgumentException for parameter/option parsing errors
Expand All @@ -314,7 +313,7 @@ public function listDatabases(array $options = [])
* @return Collection
* @throws InvalidArgumentException for parameter/option parsing errors
*/
public function selectCollection($databaseName, $collectionName, array $options = [])
public function selectCollection(string $databaseName, string $collectionName, array $options = [])
{
$options += ['typeMap' => $this->typeMap];

Expand All @@ -330,7 +329,7 @@ public function selectCollection($databaseName, $collectionName, array $options
* @return Database
* @throws InvalidArgumentException for parameter/option parsing errors
*/
public function selectDatabase($databaseName, array $options = [])
public function selectDatabase(string $databaseName, array $options = [])
{
$options += ['typeMap' => $this->typeMap];

Expand Down
21 changes: 10 additions & 11 deletions src/Collection.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,13 +126,13 @@ class Collection
* @param array $options Collection options
* @throws InvalidArgumentException for parameter/option parsing errors
*/
public function __construct(Manager $manager, $databaseName, $collectionName, array $options = [])
public function __construct(Manager $manager, string $databaseName, string $collectionName, array $options = [])
{
if (strlen((string) $databaseName) < 1) {
if (strlen($databaseName) < 1) {
throw new InvalidArgumentException('$databaseName is invalid: ' . $databaseName);
}

if (strlen((string) $collectionName) < 1) {
if (strlen($collectionName) < 1) {
throw new InvalidArgumentException('$collectionName is invalid: ' . $collectionName);
}

Expand All @@ -153,8 +153,8 @@ public function __construct(Manager $manager, $databaseName, $collectionName, ar
}

$this->manager = $manager;
$this->databaseName = (string) $databaseName;
$this->collectionName = (string) $collectionName;
$this->databaseName = $databaseName;
$this->collectionName = $collectionName;
$this->readConcern = $options['readConcern'] ?? $this->manager->getReadConcern();
$this->readPreference = $options['readPreference'] ?? $this->manager->getReadPreference();
$this->typeMap = $options['typeMap'] ?? self::$defaultTypeMap;
Expand Down Expand Up @@ -446,13 +446,13 @@ public function deleteOne($filter, array $options = [])
* @param string $fieldName Field for which to return distinct values
* @param array|object $filter Query by which to filter documents
* @param array $options Command options
* @return mixed[]
* @return array
* @throws UnexpectedValueException if the command response was malformed
* @throws UnsupportedException if options are not supported by the selected server
* @throws InvalidArgumentException for parameter/option parsing errors
* @throws DriverRuntimeException for other driver errors (e.g. connection errors)
*/
public function distinct($fieldName, $filter = [], array $options = [])
public function distinct(string $fieldName, $filter = [], array $options = [])
{
if (! isset($options['readPreference']) && ! is_in_transaction($options)) {
$options['readPreference'] = $this->readPreference;
Expand Down Expand Up @@ -938,7 +938,6 @@ public function insertOne($document, array $options = [])
* Returns information for all indexes for the collection.
*
* @see ListIndexes::__construct() for supported options
* @param array $options
* @return IndexInfoIterator
* @throws InvalidArgumentException for parameter/option parsing errors
* @throws DriverRuntimeException for other driver errors (e.g. connection errors)
Expand Down Expand Up @@ -1007,9 +1006,9 @@ public function mapReduce(JavascriptInterface $map, JavascriptInterface $reduce,
* Renames the collection.
*
* @see RenameCollection::__construct() for supported options
* @param string $toCollectionName New name of the collection
* @param ?string $toDatabaseName New database name of the collection. Defaults to the original database.
* @param array $options Additional options
* @param string $toCollectionName New name of the collection
* @param string|null $toDatabaseName New database name of the collection. Defaults to the original database.
* @param array $options Additional options
* @return array|object Command result document
* @throws UnsupportedException if options are not supported by the selected server
* @throws InvalidArgumentException for parameter/option parsing errors
Expand Down
15 changes: 5 additions & 10 deletions src/Command/ListCollections.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ class ListCollections implements Executable
* @param array $options Command options
* @throws InvalidArgumentException for parameter/option parsing errors
*/
public function __construct($databaseName, array $options = [])
public function __construct(string $databaseName, array $options = [])
{
if (isset($options['authorizedCollections']) && ! is_bool($options['authorizedCollections'])) {
throw InvalidArgumentException::invalidType('"authorizedCollections" option', $options['authorizedCollections'], 'boolean');
Expand All @@ -95,19 +95,17 @@ public function __construct($databaseName, array $options = [])
throw InvalidArgumentException::invalidType('"session" option', $options['session'], Session::class);
}

$this->databaseName = (string) $databaseName;
$this->databaseName = $databaseName;
$this->options = $options;
}

/**
* Execute the operation.
*
* @see Executable::execute()
* @param Server $server
* @return CachingIterator
* @throws DriverRuntimeException for other driver errors (e.g. connection errors)
*/
public function execute(Server $server)
public function execute(Server $server): CachingIterator
{
$cursor = $server->executeReadCommand($this->databaseName, $this->createCommand(), $this->createOptions());
$cursor->setTypeMap(['root' => 'array', 'document' => 'array']);
Expand All @@ -117,10 +115,8 @@ public function execute(Server $server)

/**
* Create the listCollections command.
*
* @return Command
*/
private function createCommand()
private function createCommand(): Command
{
$cmd = ['listCollections' => 1];

Expand All @@ -144,9 +140,8 @@ private function createCommand()
* the command be executed on the primary.
*
* @see https://php.net/manual/en/mongodb-driver-server.executecommand.php
* @return array
*/
private function createOptions()
private function createOptions(): array
{
$options = [];

Expand Down
10 changes: 3 additions & 7 deletions src/Command/ListDatabases.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,12 +99,11 @@ public function __construct(array $options = [])
* Execute the operation.
*
* @see Executable::execute()
* @param Server $server
* @return array An array of database info structures
Copy link
Member

Choose a reason for hiding this comment

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

Any reason not to use an array return type hint on execute() here? Correct me if I'm wrong, but I think that'd be permitted by covariance.

If this is out of scope for this PR, feel free to ignore.

Copy link
Member Author

Choose a reason for hiding this comment

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

Covariance would indeed allow us to add an array type here. However, backward compatibility promise won't allow us to do so, as anyone extending ListDatabases would be encountering fatal errors. Granted, it doesn't make much sense extending these classes, but they are all marked as part of the API and non-final, so we can't add return types to any public or protected methods.

Copy link
Member

Choose a reason for hiding this comment

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

Oh good point about extension. I didn't consider that.

Shame on us for not making these things final by default. Somewhere, @Ocramius is silently shaking his head in disappointment.

* @throws UnexpectedValueException if the command response was malformed
* @throws DriverRuntimeException for other driver errors (e.g. connection errors)
*/
public function execute(Server $server)
public function execute(Server $server): array
{
$cursor = $server->executeReadCommand('admin', $this->createCommand(), $this->createOptions());
$cursor->setTypeMap(['root' => 'array', 'document' => 'array']);
Expand All @@ -119,10 +118,8 @@ public function execute(Server $server)

/**
* Create the listDatabases command.
*
* @return Command
*/
private function createCommand()
private function createCommand(): Command
{
$cmd = ['listDatabases' => 1];

Expand All @@ -146,9 +143,8 @@ private function createCommand()
* the command be executed on the primary.
*
* @see https://php.net/manual/en/mongodb-driver-server.executecommand.php
* @return array
*/
private function createOptions()
private function createOptions(): array
{
$options = [];

Expand Down
Loading