PHPLIB-1118: Drop support for PHP 7.2 and 7.3 #1128

alcaeus · 2023-07-13T11:48:36Z

This drops support for PHP 7.2 and 7.3 and updates code for PHP 7.4. Changes have been made automatically using rector and phpcbf. A few manual fixes were applied (a few property types were fixed to exclude null).

alcaeus · 2023-07-13T12:01:31Z

src/Model/BSONIterator.php

-    /** @var mixed */
-    private $current;
+    /** @var array|object|null */
+    private $current = null;


Note that rector keeps changing this to private ?object $current = null;, which is incorrect as the return type of toPHP depends on the type map provided. @GromNaN is there a way to tell rector about this?

That's not satisfying, but you can assign to a local var to enforce the type.

/** @var array|object $current */ $current = toPHP(...); $this->current = $current;

It looks like we are using rector with an outdated version of phpstan stubs. https://github.com/phpstan/phpstan-src/blob/1.10.21/resources/functionMap.php#L6735

To disable the problematic rule, add this to rector.php:

$rectorConfig->skip([ // ... TypedPropertyFromAssignsRector::class => [ __DIR__ . '/src/Model/BSONIterator.php', ],

alcaeus · 2023-07-13T12:02:28Z

examples/bulk.php

@@ -68,7 +68,7 @@ function toJSON(object $document): string
                ['x' => 10], // Document
            ],
        ],
-    ]
+    ],


This change is responsible for most of the non-functional diff in this PR: a trailing comma after the last argument in multi-line method calls is now required. This is consistent with the rules already in place for multi-line arrays.

alcaeus · 2023-07-13T12:02:50Z

examples/persistable.php

@@ -16,14 +16,12 @@

 class PersistableEntry implements Persistable
 {
-    /** @var ObjectId */
-    private $id;
+    private ObjectId $id;


I went through the various type changes and confirmed they were accurate, but a second look certainly won't hurt.

Good to me. Some missing property types from ext-mongodb, maybe due to outdated stubs.

When types are added to properties, the biggest risk is to have an error when trying to access an uninitialized property.

Note: the missing property types weren't caught by Rector for some reason, and I only later realised that we had an exclusion in the phpcs config to prevent native types from being added. This has since been removed and types for properties are now enforced.

GromNaN

LGTM. With a few optional comments.

src/Collection.php

src/Database.php

GromNaN · 2023-07-13T14:25:27Z

src/Operation/CreateIndexes.php

-            return (string) $index;
-        }, $this->indexes);
+        return array_map(
+            fn (IndexInput $index): string => (string) $index,


This function is strval.

Using strval would remove the type-checking of IndexInput, but I suppose there's not much risk in doing so since we build the array in the constructor.

src/Operation/ListCollectionNames.php

GromNaN · 2023-07-13T14:29:33Z

tests/Collection/CollectionFunctionalTest.php

-                $a = json_encode($a);
-                $b = json_encode($b);
+                $a = json_encode($a, JSON_THROW_ON_ERROR);
+                $b = json_encode($b, JSON_THROW_ON_ERROR);


Good improvement. Even if that doesn't change anything for the test.

tests/UnifiedSpecTests/UnifiedTestRunner.php

src/Collection.php

src/Database.php

src/GridFS/Bucket.php

src/MapReduceResult.php

jmikola · 2023-07-13T17:56:11Z

src/Operation/CreateIndexes.php

-            return (string) $index;
-        }, $this->indexes);
+        return array_map(
+            fn (IndexInput $index): string => (string) $index,


Using strval would remove the type-checking of IndexInput, but I suppose there's not much risk in doing so since we build the array in the constructor.

tests/SpecTests/Operation.php

tests/SpecTests/RetryableReadsSpecTest.php

tests/SpecTests/RetryableWritesSpecTest.php

tests/UnifiedSpecTests/RunOnRequirement.php

tests/UnifiedSpecTests/UnifiedTestCase.php

This uses rector and phpcbf to automatically fix issues

jmikola

LGTM after the necessary change in EntityMap.php.

Everything else is suggestions, for which I'll defer to you.

tests/SpecTests/RetryableWritesSpecTest.php

src/Operation/Aggregate.php

src/Operation/InsertMany.php

jmikola · 2023-07-14T13:41:27Z

tests/SpecTests/ErrorExpectation.php

-     */
-    private static $codeNameMap = [
+    /** @see https://github.com/mongodb/mongo/blob/master/src/mongo/base/error_codes.err */
+    private static array $codeNameMap = [


I just realized that the private static constants under tests/ were never addressed in c0738eb. Not sure if that was intentional on @GromNaN's part, but I don't remember it coming up in the PR.

Not sure why those weren't addressed, I'll let @GromNaN follow up on this.

tests/SpecTests/ErrorExpectation.php

tests/UnifiedSpecTests/EntityMap.php

alcaeus force-pushed the phplib-1118-drop-old-php branch 3 times, most recently from 0416b94 to 3f08b46 Compare July 13, 2023 12:28

alcaeus commented Jul 13, 2023

View reviewed changes

alcaeus force-pushed the phplib-1118-drop-old-php branch from 3f08b46 to 14d6d65 Compare July 13, 2023 12:44

alcaeus requested review from GromNaN and jmikola July 13, 2023 13:13

alcaeus marked this pull request as ready for review July 13, 2023 13:13

GromNaN approved these changes Jul 13, 2023

View reviewed changes

jmikola reviewed Jul 13, 2023

View reviewed changes

alcaeus force-pushed the phplib-1118-drop-old-php branch from 14d6d65 to 6eee5ec Compare July 14, 2023 07:07

alcaeus added 8 commits July 14, 2023 09:34

PHPLIB-1118: Drop support for PHP 7.2 and 7.3

65ee703

Modernise code for PHP 7.4

c9abfb9

This uses rector and phpcbf to automatically fix issues

Skip rector rule creating adding wrong property type

99c2d47

Add more missing property types

fbb04e4

Use strval instead of closure

8798813

Use FQN for constant in documentation examples

75d434d

Add undectected missing trailing commas

fb6ff9a

Split line for readability

a39cc24

alcaeus force-pushed the phplib-1118-drop-old-php branch from 6eee5ec to b8acdd1 Compare July 14, 2023 07:37

Fix wrong property type

17f395a

alcaeus force-pushed the phplib-1118-drop-old-php branch from b8acdd1 to 17f395a Compare July 14, 2023 07:46

Remove requirement for JSON_THROW_ON_ERROR in docs examples

ebf877e

alcaeus requested a review from jmikola July 14, 2023 08:52

jmikola approved these changes Jul 14, 2023

View reviewed changes

Address code review issues

e6c2595

alcaeus merged commit f81b4a6 into mongodb:master Jul 14, 2023

alcaeus deleted the phplib-1118-drop-old-php branch July 14, 2023 14:48

GromNaN mentioned this pull request Sep 20, 2023

PHP 7.3 polyfill is not necessary because PHP 7.4 is required #1167

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PHPLIB-1118: Drop support for PHP 7.2 and 7.3 #1128

PHPLIB-1118: Drop support for PHP 7.2 and 7.3 #1128

alcaeus commented Jul 13, 2023

alcaeus Jul 13, 2023

GromNaN Jul 13, 2023

GromNaN Jul 13, 2023

GromNaN Jul 13, 2023

alcaeus Jul 13, 2023 •

edited

Loading

alcaeus Jul 13, 2023

GromNaN Jul 13, 2023

alcaeus Jul 14, 2023

GromNaN left a comment

GromNaN Jul 13, 2023

jmikola Jul 13, 2023

GromNaN Jul 13, 2023

jmikola Jul 13, 2023

jmikola left a comment

jmikola Jul 14, 2023

alcaeus Jul 14, 2023

PHPLIB-1118: Drop support for PHP 7.2 and 7.3 #1128

PHPLIB-1118: Drop support for PHP 7.2 and 7.3 #1128

Conversation

alcaeus commented Jul 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alcaeus Jul 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GromNaN left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmikola left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alcaeus Jul 13, 2023 •

edited

Loading