Switch to phpcs and upgrade codebase #2596

GromNaN · 2023-09-04T09:14:16Z

To be consistent with other MongoDB PHP projects, we'll use phpcs to analyze and fix the code style.

Summary of changes:

added "declare(strict_types=1);" to all file headers
added "use function" for all functions used in each file (performance gain for functions with specific opcode compilation)
use of short closures
remove @var annotations in tests, replaced by assertInstanceOf or assert
Use early exit when appropriate, but disable the rule
Remove @param & @return phpdoc when duplicate of native types
~~Added argument and return types from phpdoc. Removal of redundant phpdocs.~~

GromNaN

A lot of errors to solve.
Arg types cannot be added until Laravel adds them.

src/Collection.php

src/Connection.php

src/Eloquent/HybridRelations.php

alcaeus

First pass through the src folder. Tests will follow later.

phpcs.xml.dist

alcaeus · 2023-09-05T06:49:59Z

src/Concerns/ManagesTransactions.php

 trait ManagesTransactions
 {
-    protected ?Session $session = null;
+    protected Session|null $session = null;


There's a phpcs rule that enforces the union type syntax for nullable types. I personally prefer the more concise ?<type> syntax for this. If you want to continue using ?<type>, exclude the SlevomatCodingStandard.TypeHints.UnionTypeHintFormat.DisallowedShortNullable rule.

alcaeus · 2023-09-05T06:51:48Z

src/Collection.php

@@ -56,21 +58,23 @@ public function __call(string $method, array $parameters)

        // Convert the query parameters to a json string.
        array_walk_recursive($parameters, function (&$item, $key) {
-            if ($item instanceof ObjectID) {
-                $item = (string) $item;
+            if (! ($item instanceof ObjectID)) {


The Doctrine Coding Standard enforces an "early exit" rule, meaning that all conditionals must be written to return early to reduce code nesting. It can be useful, but I agree that it doesn't help in this case. You can either add a @phpcs-ignore comment above the line, or remove the rule entirely if @jmikola has no objections.

Adding // @phpcs:ignore SlevomatCodingStandard.ControlStructures.EarlyExit.EarlyExitNotUsed

Instead, I excluded this rule globally.

alcaeus · 2023-09-05T06:52:47Z

src/Connection.php

-            if (preg_match('/^mongodb(?:[+]srv)?:\\/\\/.+\\/([^?&]+)/s', $dsn, $matches)) {
-                $config['database'] = $matches[1];
-            } else {
+            if (! preg_match('/^mongodb(?:[+]srv)?:\\/\\/.+\\/([^?&]+)/s', $dsn, $matches)) {


Note: this is the kind of change made by the "early exit" rule - usually it reduces complexity of the code.

It's OK for this.

src/Connection.php

src/Query/Builder.php

alcaeus · 2023-09-05T07:08:34Z

src/Query/Builder.php

@@ -982,9 +1013,11 @@ protected function compileWheres(): array
            } elseif (isset($where['values'])) {
                if (is_array($where['values'])) {
                    array_walk_recursive($where['values'], function (&$item, $key) {
-                        if ($item instanceof DateTimeInterface) {
-                            $item = new UTCDateTime($item);
+                        if (! ($item instanceof DateTimeInterface)) {


Suggested change

if (! ($item instanceof DateTimeInterface)) {

if (! $item instanceof DateTimeInterface) {

Reverted early exit and added phpcs:ignore

src/Queue/MongoQueue.php

alcaeus · 2023-09-05T07:11:39Z

src/Relations/EmbedsOneOrMany.php

@@ -195,35 +203,31 @@ protected function getIdsArrayFrom($ids)
        }

        foreach ($ids as &$id) {
-            if ($id instanceof Model) {
-                $id = $id->getKey();
+            if (! ($id instanceof Model)) {


Suggested change

if (! ($id instanceof Model)) {

if (! $id instanceof Model) {

src/Schema/Builder.php

GromNaN

Thanks for the review. I added excluded rules and made a lot of necessary manual changes to please phpcs.

GromNaN · 2023-09-05T13:42:41Z

src/Connection.php

-            if (preg_match('/^mongodb(?:[+]srv)?:\\/\\/.+\\/([^?&]+)/s', $dsn, $matches)) {
-                $config['database'] = $matches[1];
-            } else {
+            if (! preg_match('/^mongodb(?:[+]srv)?:\\/\\/.+\\/([^?&]+)/s', $dsn, $matches)) {


It's OK for this.

GromNaN · 2023-09-05T13:44:15Z

src/Collection.php

@@ -56,21 +58,23 @@ public function __call(string $method, array $parameters)

        // Convert the query parameters to a json string.
        array_walk_recursive($parameters, function (&$item, $key) {
-            if ($item instanceof ObjectID) {
-                $item = (string) $item;
+            if (! ($item instanceof ObjectID)) {


Adding // @phpcs:ignore SlevomatCodingStandard.ControlStructures.EarlyExit.EarlyExitNotUsed

src/Eloquent/Model.php

GromNaN · 2023-09-05T14:05:45Z

src/Eloquent/Model.php

-                    $relations[] = $key.'.'.$entityValue;
-                }
+            if (! ($relation instanceof QueueableEntity)) {
+                continue;


Adding phpcs:ignore here

GromNaN · 2023-09-05T14:37:16Z

src/Query/Builder.php

@@ -880,7 +910,7 @@ protected function performUpdate($query, array $options = [])

        $wheres = $this->compileWheres();
        $result = $this->collection->updateMany($wheres, $query, $options);
-        if (1 == (int) $result->isAcknowledged()) {
+        if ((int) $result->isAcknowledged() === 1) {


Done for the the file.

src/Query/Builder.php

GromNaN · 2023-09-05T14:41:46Z

src/Query/Builder.php

@@ -982,9 +1013,11 @@ protected function compileWheres(): array
            } elseif (isset($where['values'])) {
                if (is_array($where['values'])) {
                    array_walk_recursive($where['values'], function (&$item, $key) {
-                        if ($item instanceof DateTimeInterface) {
-                            $item = new UTCDateTime($item);
+                        if (! ($item instanceof DateTimeInterface)) {


Reverted early exit and added phpcs:ignore

src/Schema/Builder.php

alcaeus

There are a few more phpcs violation that should be fixable using phpcbf, other than that current changes look good.

src/Relations/EmbedsMany.php

GromNaN · 2023-09-06T12:08:56Z

I forgot running phpcbf after the last rebase. Fixed.

alcaeus

LGTM 🤞

GromNaN requested review from jmikola and alcaeus September 4, 2023 09:14

GromNaN marked this pull request as draft September 4, 2023 09:29

GromNaN force-pushed the phpcs branch from e183478 to 7c82328 Compare September 4, 2023 19:48

GromNaN commented Sep 4, 2023

View reviewed changes

alcaeus reviewed Sep 5, 2023

View reviewed changes

GromNaN force-pushed the phpcs branch from 7a84a88 to 900afa7 Compare September 5, 2023 17:14

GromNaN commented Sep 5, 2023

View reviewed changes

GromNaN marked this pull request as ready for review September 6, 2023 06:44

GromNaN force-pushed the phpcs branch from 900afa7 to febec4f Compare September 6, 2023 09:20

alcaeus reviewed Sep 6, 2023

View reviewed changes

src/Relations/EmbedsMany.php Show resolved Hide resolved

GromNaN force-pushed the phpcs branch from febec4f to 5bb3e4d Compare September 6, 2023 12:06

Switch to phpcs and upgrade codebase

bd4be33

GromNaN force-pushed the phpcs branch from 5bb3e4d to bd4be33 Compare September 6, 2023 12:07

GromNaN requested a review from alcaeus September 6, 2023 12:12

alcaeus approved these changes Sep 6, 2023

View reviewed changes

GromNaN merged commit 9bc8999 into mongodb:master Sep 6, 2023

GromNaN deleted the phpcs branch September 6, 2023 12:48

	if (! ($item instanceof DateTimeInterface)) {
	if (! $item instanceof DateTimeInterface) {

	if (! ($id instanceof Model)) {
	if (! $id instanceof Model) {

Switch to phpcs and upgrade codebase #2596

Switch to phpcs and upgrade codebase #2596

Uh oh!

Conversation

GromNaN commented Sep 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GromNaN left a comment • 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alcaeus left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

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!

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!

Uh oh!

alcaeus left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

GromNaN commented Sep 6, 2023

Uh oh!

alcaeus left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

GromNaN commented Sep 4, 2023 •

edited

Loading

GromNaN left a comment •

edited

Loading