PHPLIB-955: Require PHP 8.1 #1374

alcaeus · 2024-09-04T13:47:33Z

The first commit updates the requirement in composer.json and updates CI configs. In the second commit, I applied all automated fixes from phpcbf. This mainly affects the use of union types as well as the mixed type.

A couple of notes on phpcs rules:

The "DisallowedShortNullable" rule was disabled, as it would change all ?Type types to Type|null. I prefer the short syntax over the consistency with other union types (e.g. Type|Other|null)
The rule requiring constructor property promotion was disabled as it caused some invalid syntax, for example by adding a default value to a constructor parameter when other parameters after it don't have a default value. I'll investigate and create a bug ticket for this.

alcaeus · 2024-09-05T07:54:14Z

Marking this ready for review. I'll revisit the failing tests shortly, as I suspect those will be fixed by #1373

alcaeus · 2024-09-05T07:55:20Z

tests/Operation/MapReduceTest.php

@@ -1,5 +1,7 @@
 <?php

+declare(strict_types=1);


This directive is necessary to disable type coercion in testConstructorOutArgumentTypeCheck; all of the invalid values provided there can be coerced to strings.

This means that the validation is weaker than before, since the values were not cast. Do we consider that it's the user's responsibility to send the right type and use strict mode if they wish to avoid this kind of error? In any case, it won't work with an int casted to string.

Before this change, users could pass an int which we'd have to guard against. With the string type enforced by PHP, there are two scenarios: if the calling file has strict_types disabled, an int would be coerced to string; with strict_types enabled a TypeError is thrown. The end result is the same: the method will not be called with a non-string parameter.

GromNaN · 2024-09-05T07:45:42Z

src/Codec/DecodeIfSupported.php

     * @psalm-param BSONType $value
     * @return mixed
     * @psalm-return NativeType
     * @throws UnsupportedValueException if the decoder does not support the value
     */
-    abstract public function decode($value);
+    abstract public function decode(mixed $value);


Confirmed this is not a breaking change for classes that override this abstract method. https://3v4l.org/donQG

I don't get why Symfony BC promise prohibit adding typehint to arguments in classes. https://symfony.com/doc/current/contributing/code/bc.html#changing-classes

Correct. Without a type specified for an argument, people can't add their own, narrower types. When we add a type, people are still allowed to widen types or omit the type entirely, which is why we can add parameter types in classes and interfaces in minor versions.

/me looks over at the Covariance and Contravariance poster hanging on my wall.

GromNaN · 2024-09-05T07:49:48Z

src/GridFS/StreamWrapper.php

@@ -54,8 +54,7 @@ class StreamWrapper
    /** @var resource|null Stream context (set by PHP) */
    public $context;

-    /** @var ReadableStream|WritableStream|null */
-    private $stream;
+    private ReadableStream|WritableStream|null $stream = null;


Adding the type on properties produces an error in case of access to uninitialized property. That's why you added the default null value here.

That's correct 👍

When does PHP raise an error for the uninitialized property? I'm curious if the error is only triggered by explicitly assigning null in the property declaration. If so, would you rather do without that and omit |null from the type?

See: https://3v4l.org/RHWMH#v8.3.11

If the type was only ReadableStream|WritableStream without a default value, accessing $this->stream would cause a fatal error. I believe we could theoretically use that instead of assert in a number of places, as even with disabled assertion handling any access to the stream would result in an object access on null, which again yields a fatal error.

I'll note that isset does not trigger the uninitialised property error, so we would still be able to check if a stream was initialised. I can change this if y'all prefer to leave null out of the type.

To add on to this, I'll note that psalm does not like properties without a default value that aren't initialised in the constructor:

ERROR: MissingConstructor - src/GridFS/StreamWrapper.php:56:43 - MongoDB\GridFS\StreamWrapper has an uninitialized property MongoDB\GridFS\StreamWrapper::$stream, but no constructor (see https://psalm.dev/073) private ReadableStream|WritableStream $stream;

Feel free to leave as-is. Seems like the path of least resistance.

src/Exception/UnsupportedValueException.php

GromNaN · 2024-09-05T07:54:22Z

tests/TestCase.php

@@ -250,7 +243,8 @@ protected function getInvalidBooleanValues(bool $includeNull = false): array
     */
    protected function getInvalidDocumentValues(bool $includeNull = false): array
    {
-        return array_merge([123, 3.14, 'foo', true, PackedArray::fromPHP([])], $includeNull ? [null] : []);
+        return array_merge([123, 3.14, 'foo', true], $includeNull ? [null] : []);
+//        return array_merge([123, 3.14, 'foo', true, PackedArray::fromPHP([])], $includeNull ? [null] : []);


You left this commented.

Whoops, removed.

For context, with the object|array type now enforced for documents, PackedArray instances no longer cause an error. I could bring back the PackedArray check if we want to be more specific about what objects are allowed, but I defer to @jmikola on this.

Isn't the PackedArray check still in place? I saw you removed various conditionals when adding array|object type hints, but is_document() still rejects PackedArray instances. I would expect that that InvalidArgumentException::expectedDocumentType() still gets thrown for PackedArray instances.

I assume changing InvalidArgumentException expectations to TypeError is what conflicts here. I don't feel strongly about this, but I'd argue that parameter and option type checks shouldn't necessarily be expected to raise TypeError. There are still various other contexts where we raise InvalidArgumentException for validation errors (e.g. option value is out of range, or a string value doesn't satisfy our homemade enum).

It is indeed. I've updated the type assertions based on the value given to expect a TypeError or InvalidArgumentException and restored the PackedArray instance as an invalid document, as it adds confidence that we have the right checks in place.

With TypeError being a thing, I wonder if we should consider throwing TypeError throwables instead of InvalidArgumentException on type mismatches.

With TypeError being a thing, I wonder if we should consider throwing TypeError throwables instead of InvalidArgumentException on type mismatches.

That sounds like a great idea, now that we can reliably expect TypeErrors from PHP itself. Might be something to consider for both PHPC and PHPLIB, and then we can also remove the vapid documentation for throwing InvalidArgumentException from most things.

tests/UnifiedSpecTests/EventObserver.php

GromNaN

LGTM

GromNaN · 2024-09-05T09:57:49Z

psalm-baseline.xml

    </MoreSpecificReturnType>
  </file>
+  <file src="src/Model/BSONIterator.php">
+    <MixedArgument>
+      <code><![CDATA[$this->options['typeMap']]]></code>


Should we define array shape for the options?

We had that. However, in the param declaration, it would have to be array{typeMap?: array, ...}, whereas for the property it's array{typeMap: array, ...}. With promoted properties, the same type is used for the parameter and property, leading to this error. Instead of adding custom logic to work around this, I decided to add the error to the baseline.

src/Model/IndexInput.php

src/Operation/CreateEncryptedCollection.php

jmikola · 2024-09-05T15:36:03Z

src/Client.php

@@ -397,7 +397,7 @@ private static function getVersion(): string
        if (self::$version === null) {
            try {
                self::$version = InstalledVersions::getPrettyVersion('mongodb/mongodb') ?? 'unknown';
-            } catch (Throwable $t) {
+            } catch (Throwable) {


Noted that the variable is no longer required as of PHP 8.0 (see: Exceptions).

jmikola · 2024-09-05T15:55:12Z

tests/TestCase.php

@@ -250,7 +243,8 @@ protected function getInvalidBooleanValues(bool $includeNull = false): array
     */
    protected function getInvalidDocumentValues(bool $includeNull = false): array
    {
-        return array_merge([123, 3.14, 'foo', true, PackedArray::fromPHP([])], $includeNull ? [null] : []);
+        return array_merge([123, 3.14, 'foo', true], $includeNull ? [null] : []);
+//        return array_merge([123, 3.14, 'foo', true, PackedArray::fromPHP([])], $includeNull ? [null] : []);


Isn't the PackedArray check still in place? I saw you removed various conditionals when adding array|object type hints, but is_document() still rejects PackedArray instances. I would expect that that InvalidArgumentException::expectedDocumentType() still gets thrown for PackedArray instances.

I assume changing InvalidArgumentException expectations to TypeError is what conflicts here. I don't feel strongly about this, but I'd argue that parameter and option type checks shouldn't necessarily be expected to raise TypeError. There are still various other contexts where we raise InvalidArgumentException for validation errors (e.g. option value is out of range, or a string value doesn't satisfy our homemade enum).

alcaeus requested a review from GromNaN September 5, 2024 07:53

alcaeus marked this pull request as ready for review September 5, 2024 07:53

alcaeus requested a review from a team as a code owner September 5, 2024 07:53

alcaeus commented Sep 5, 2024

View reviewed changes

GromNaN reviewed Sep 5, 2024

View reviewed changes

alcaeus requested review from jmikola, GromNaN and a team September 5, 2024 07:55

GromNaN approved these changes Sep 5, 2024

View reviewed changes

github-advanced-security bot found potential problems Sep 5, 2024

View reviewed changes

src/Model/IndexInput.php Dismissed Show dismissed Hide dismissed

src/Operation/CreateEncryptedCollection.php Dismissed Show dismissed Hide dismissed

jmikola reviewed Sep 5, 2024

View reviewed changes

alcaeus force-pushed the phplib-955-require-php-8.1 branch from bf2e611 to 48c14de Compare September 6, 2024 09:32

alcaeus requested a review from jmikola September 6, 2024 11:54

jmikola approved these changes Sep 6, 2024

View reviewed changes

alcaeus added 12 commits September 9, 2024 08:15

PHPLIB-955: Require PHP 8.1

8acbed9

Apply automated phpcs fixes for PHP 8.1

4b63893

Fix psalm issues

78b4719

Remove unnecessary type assertions

f92fbe7

Fix wrong type in DocumentsMatchConstraint

f725dca

Use constructor property promotion where possible

8369be1

Update Psalm baseline

9c0da5c

Remove commented code

c52905e

Use match statement instead of switch

213a44c

Fix breakage introduced by constructor property promotion

8a3981e

Always access promoted options property in constructor

1b70949

Restore PackedArray as invalid document in tests

a44c71f

alcaeus force-pushed the phplib-955-require-php-8.1 branch from 48c14de to a44c71f Compare September 9, 2024 06:15

alcaeus enabled auto-merge (squash) September 9, 2024 06:15

alcaeus merged commit 24214b6 into mongodb:master Sep 9, 2024
30 checks passed

alcaeus deleted the phplib-955-require-php-8.1 branch September 9, 2024 06:36

PHPLIB-955: Require PHP 8.1 #1374

PHPLIB-955: Require PHP 8.1 #1374

Uh oh!

Conversation

alcaeus commented Sep 4, 2024

Uh oh!

alcaeus commented Sep 5, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmikola Sep 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

GromNaN left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jmikola Sep 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jmikola Sep 5, 2024 •

edited

Loading

jmikola Sep 5, 2024 •

edited

Loading