PHPLIB-599: Typing improvements #959
Conversation
alcaeus
force-pushed
the
feature/phplib-599-typing
branch
from
a2e2ee7 to
68d68e3
Compare
| @@ -105,8 +105,6 @@ | |||
| <!-- **************************************************************************** --> | |||
| <rule ref="SlevomatCodingStandard.TypeHints.ParameterTypeHint"> | |||
| <properties> | |||
| <!-- Requires PHP 7.4 --> | |||
| <property name="enableObjectTypeHint" value="false" /> | |||
There was a problem hiding this comment.
The comment was wrong - the object type was introduced in PHP 7.2.
| // TODO: Move to unit test once ManagerInterface can be mocked (PHPC-378) | ||
| new Collection($this->manager, $this->getDatabaseName(), $collectionName); | ||
| } | ||
|
|
||
| public function provideInvalidDatabaseAndCollectionNames() | ||
| { | ||
| return [ | ||
| [null], | ||
| [''], | ||
| [null, TypeError::class], |
There was a problem hiding this comment.
Adding the expected exception type to the dataset was necessary as null is now caught by PHP itself, while the empty string is caught by our logic.
It was a mix of |
alcaeus
force-pushed
the
feature/phplib-599-typing
branch
from
0548d90 to
00a8eec
Compare
alcaeus
force-pushed
the
feature/phplib-599-typing
branch
from
00a8eec to
1dec7b2
Compare
There was a problem hiding this comment.
getCommandDocument and execute() methods in Operations can probably get their doc blocks stripped down and return types added in some places. I think covariance on the interface allows for that.
@param annotations with single types (e.g. array, mixed) that don't have a description (or a meaningful description) of the parameter can probably be removed. The single type is probably already in the method signature. Likewise for |null stuff.
Likewise for @return annotations.
Lastly, I understand if you'd like to avoid adding type hints to the Executable and Explainable interfaces, even though they're documented as @internal. Perhaps we should have a ticket to add type hints and schedule it for 2.0, though.
| @@ -99,7 +99,6 @@ public function __construct(array $options = []) | |||
| * Execute the operation. | |||
| * | |||
| * @see Executable::execute() | |||
| * @param Server $server | |||
| * @return array An array of database info structures | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
@jmikola you are correct - it looks like tooling did indeed leave most (if not all) As for Last but not least, most of our classes are neither |
* PHPLIB-600: Add type definitions for parameters * PHPLIB-602: Add return type definitions where applicable * Enable object type for parameters * Add comment about using declare * Remove useless param and return annotations * Remove redundant type casts * Add return types to internal classes * Use object consistently in GridFS classes * Remove useless type annotations * Remove unnecessary cast
This PR handles PHPLIB-600 and PHPLIB-602, adding parameter types and return types where applicable.
Rules for phpcs have been updated to require argument types; no changes were made to return types as most of the classes are non-final (something we may want to address for 2.0).