Merge branch '2.2.x' into 2.3.x

* 2.2.x: Fix handling of upserts during scheduling for deletion (doctrine#2334) Fix wrong handling for nullable fields in upsert and update (doctrine#2318) [2.2] Fix builds (doctrine#2319) Allow mixed value in $not operator (doctrine#2307) Fix errors with nullable typed associations (doctrine#2302) Fix using null values in partial filter expressions (doctrine#2300) Fix preparation of $elemMatch operators in queries (doctrine#2298) Fix query preparation when in elemMatch (doctrine#2299) Fix mapping of the nullable option for XML driver Fix documentation for uploadFromFile Correctly handle write concern specified in defaultCommitOptions (doctrine#2294) Fix invalid strict comparison when validating mappings Update storage-strategies.rst Update working-with-objects.rst Test serialization of lock/version fields Fix locking when ClassMetadata is unserialized
alcaeus · Jun 29, 2021 · b6f3f32 · b6f3f32
2 parents 4d34a81 + f20b3cd
commit b6f3f32
Show file tree

Hide file tree

Showing 19 changed files with 426 additions and 73 deletions.
diff --git a/.github/workflows/coding-standards.yml b/.github/workflows/coding-standards.yml
@@ -50,6 +50,11 @@ jobs:
       - name: "Install dependencies with Composer"
         uses: "ramsey/composer-install@v1"
 
+      - name: "Upload composer.lock as build artifact"
+        uses: actions/upload-artifact@v2
+        with:
+          name: composer.lock
+          path: composer.lock
 
       # https://github.com/doctrine/.github/issues/3
       - name: "Run PHP_CodeSniffer"

diff --git a/.github/workflows/continuous-integration.yml b/.github/workflows/continuous-integration.yml
@@ -12,12 +12,10 @@ env:
 jobs:
   phpunit:
     name: "PHPUnit"
-    runs-on: "${{ matrix.os }}"
+    runs-on: "ubuntu-18.04"
 
     strategy:
       matrix:
-        os:
-          - "ubuntu-18.04"
         php-version:
           - "7.2"
           - "7.3"
@@ -35,13 +33,11 @@ jobs:
           - "highest"
         include:
           - deps: "lowest"
-            os: "ubuntu-16.04"
             php-version: "7.2"
             mongodb-version: "3.6"
             driver-version: "1.5.0"
             topology: "server"
           - topology: "sharded_cluster"
-            os: "ubuntu-18.04"
             php-version: "8.0"
             mongodb-version: "4.4"
             driver-version: "stable"
@@ -86,6 +82,12 @@ jobs:
           dependency-versions: "${{ matrix.dependencies }}"
           composer-options: "--prefer-dist"
 
+      - name: "Upload composer.lock as build artifact"
+        uses: actions/upload-artifact@v2
+        with:
+          name: composer.lock
+          path: composer.lock
+
       - id: setup-mongodb
         uses: mongodb-labs/drivers-evergreen-tools@master
         with:

diff --git a/.github/workflows/performance.yml b/.github/workflows/performance.yml
@@ -54,6 +54,12 @@ jobs:
       - name: "Install dependencies with Composer"
         uses: "ramsey/composer-install@v1"
 
+      - name: "Upload composer.lock as build artifact"
+        uses: actions/upload-artifact@v2
+        with:
+          name: composer.lock
+          path: composer.lock
+
       # https://github.com/doctrine/.github/issues/3
       - name: "Run PHP_CodeSniffer"
         run: "vendor/bin/phpbench run --report=default --revs=100 --iterations=5 --report=aggregate"
diff --git a/.github/workflows/static-analysis.yml b/.github/workflows/static-analysis.yml
@@ -49,6 +49,12 @@ jobs:
       - name: "Install dependencies with Composer"
         uses: "ramsey/composer-install@v1"
 
+      - name: "Upload composer.lock as build artifact"
+        uses: actions/upload-artifact@v2
+        with:
+          name: composer.lock
+          path: composer.lock
+
       - name: "Run a static analysis with phpstan/phpstan"
         run: "vendor/bin/phpstan analyse --error-format=github"
 
@@ -75,5 +81,11 @@ jobs:
       - name: "Install dependencies with Composer"
         uses: "ramsey/composer-install@v1"
 
+      - name: "Upload composer.lock as build artifact"
+        uses: actions/upload-artifact@v2
+        with:
+          name: composer.lock
+          path: composer.lock
+
       - name: "Run a static analysis with vimeo/psalm"
         run: "vendor/bin/psalm --show-info=false --stats --output-format=github --threads=$(nproc) --php-version=${{ matrix.php-version }}"
diff --git a/lib/Doctrine/ODM/MongoDB/Persisters/PersistenceBuilder.php b/lib/Doctrine/ODM/MongoDB/Persisters/PersistenceBuilder.php
@@ -68,14 +68,11 @@ public function prepareInsertData($document)
         foreach ($class->fieldMappings as $mapping) {
             $new = $changeset[$mapping['fieldName']][1] ?? null;
 
-            if ($new === null && $mapping['nullable']) {
-                $insertData[$mapping['name']] = null;
-            }
-
-            /* Nothing more to do for null values, since we're either storing
-             * them (if nullable was true) or not.
-             */
             if ($new === null) {
+                if ($mapping['nullable']) {
+                    $insertData[$mapping['name']] = null;
+                }
+
                 continue;
             }
 
@@ -143,34 +140,36 @@ public function prepareUpdateData($document)
 
             [$old, $new] = $change;
 
+            if ($new === null) {
+                if ($mapping['nullable'] === true) {
+                    $updateData['$set'][$mapping['name']] = null;
+                } else {
+                    $updateData['$unset'][$mapping['name']] = true;
+                }
+
+                continue;
+            }
+
             // Scalar fields
             if (! isset($mapping['association'])) {
-                if ($new === null && $mapping['nullable'] !== true) {
-                    $updateData['$unset'][$mapping['name']] = true;
+                if (isset($mapping['strategy']) && $mapping['strategy'] === ClassMetadata::STORAGE_STRATEGY_INCREMENT) {
+                    $operator = '$inc';
+                    $type     = Type::getType($mapping['type']);
+                    assert($type instanceof Incrementable);
+                    $value = $type->convertToDatabaseValue($type->diff($old, $new));
                 } else {
-                    if ($new !== null && isset($mapping['strategy']) && $mapping['strategy'] === ClassMetadata::STORAGE_STRATEGY_INCREMENT) {
-                        $operator = '$inc';
-                        $type     = Type::getType($mapping['type']);
-                        assert($type instanceof Incrementable);
-                        $value = $type->convertToDatabaseValue($type->diff($old, $new));
-                    } else {
-                        $operator = '$set';
-                        $value    = $new === null ? null : Type::getType($mapping['type'])->convertToDatabaseValue($new);
-                    }
-
-                    $updateData[$operator][$mapping['name']] = $value;
+                    $operator = '$set';
+                    $value    = Type::getType($mapping['type'])->convertToDatabaseValue($new);
                 }
 
+                $updateData[$operator][$mapping['name']] = $value;
+
             // @EmbedOne
             } elseif (isset($mapping['association']) && $mapping['association'] === ClassMetadata::EMBED_ONE) {
                 // If we have a new embedded document then lets set the whole thing
-                if ($new && $this->uow->isScheduledForInsert($new)) {
+                if ($this->uow->isScheduledForInsert($new)) {
                     $updateData['$set'][$mapping['name']] = $this->prepareEmbeddedDocumentValue($mapping, $new);
 
-                // If we don't have a new value then lets unset the embedded document
-                } elseif (! $new) {
-                    $updateData['$unset'][$mapping['name']] = true;
-
                 // Update existing embedded document
                 } else {
                     $update = $this->prepareUpdateData($new);
@@ -182,7 +181,7 @@ public function prepareUpdateData($document)
                 }
 
             // @ReferenceMany, @EmbedMany
-            } elseif (isset($mapping['association']) && $mapping['type'] === 'many' && $new) {
+            } elseif (isset($mapping['association']) && $mapping['type'] === 'many') {
                 if (CollectionHelper::isAtomic($mapping['strategy']) && $this->uow->isCollectionScheduledForUpdate($new)) {
                     $updateData['$set'][$mapping['name']] = $this->prepareAssociatedCollectionValue($new, true);
                 } elseif (CollectionHelper::isAtomic($mapping['strategy']) && $this->uow->isCollectionScheduledForDeletion($new)) {
@@ -208,11 +207,7 @@ public function prepareUpdateData($document)
 
             // @ReferenceOne
             } elseif (isset($mapping['association']) && $mapping['association'] === ClassMetadata::REFERENCE_ONE) {
-                if (isset($new) || $mapping['nullable'] === true) {
-                    $updateData['$set'][$mapping['name']] = $new === null ? null : $this->prepareReferencedDocumentValue($mapping, $new);
-                } else {
-                    $updateData['$unset'][$mapping['name']] = true;
-                }
+                $updateData['$set'][$mapping['name']] = $this->prepareReferencedDocumentValue($mapping, $new);
             }
         }
 
@@ -250,31 +245,36 @@ public function prepareUpsertData($document)
 
             [$old, $new] = $change;
 
+            // Fields with a null value should only be written for inserts
+            if ($new === null) {
+                if ($mapping['nullable'] === true) {
+                    $updateData['$setOnInsert'][$mapping['name']] = null;
+                }
+
+                continue;
+            }
+
             // Scalar fields
             if (! isset($mapping['association'])) {
-                if ($new !== null) {
-                    if (empty($mapping['id']) && isset($mapping['strategy']) && $mapping['strategy'] === ClassMetadata::STORAGE_STRATEGY_INCREMENT) {
-                        $operator = '$inc';
-                        $type     = Type::getType($mapping['type']);
-                        assert($type instanceof Incrementable);
-                        $value = $type->convertToDatabaseValue($type->diff($old, $new));
-                    } else {
-                        $operator = '$set';
-                        $value    = Type::getType($mapping['type'])->convertToDatabaseValue($new);
-                    }
-
-                    $updateData[$operator][$mapping['name']] = $value;
-                } elseif ($mapping['nullable'] === true) {
-                    $updateData['$setOnInsert'][$mapping['name']] = null;
+                if (empty($mapping['id']) && isset($mapping['strategy']) && $mapping['strategy'] === ClassMetadata::STORAGE_STRATEGY_INCREMENT) {
+                    $operator = '$inc';
+                    $type     = Type::getType($mapping['type']);
+                    assert($type instanceof Incrementable);
+                    $value = $type->convertToDatabaseValue($type->diff($old, $new));
+                } else {
+                    $operator = '$set';
+                    $value    = Type::getType($mapping['type'])->convertToDatabaseValue($new);
                 }
 
+                $updateData[$operator][$mapping['name']] = $value;
+
             // @EmbedOne
             } elseif (isset($mapping['association']) && $mapping['association'] === ClassMetadata::EMBED_ONE) {
                 // If we don't have a new value then do nothing on upsert
                 // If we have a new embedded document then lets set the whole thing
-                if ($new && $this->uow->isScheduledForInsert($new)) {
+                if ($this->uow->isScheduledForInsert($new)) {
                     $updateData['$set'][$mapping['name']] = $this->prepareEmbeddedDocumentValue($mapping, $new);
-                } elseif ($new) {
+                } else {
                     // Update existing embedded document
                     $update = $this->prepareUpsertData($new);
                     foreach ($update as $cmd => $values) {
@@ -286,9 +286,7 @@ public function prepareUpsertData($document)
 
             // @ReferenceOne
             } elseif (isset($mapping['association']) && $mapping['association'] === ClassMetadata::REFERENCE_ONE) {
-                if (isset($new) || $mapping['nullable'] === true) {
-                    $updateData['$set'][$mapping['name']] = $new === null ? null : $this->prepareReferencedDocumentValue($mapping, $new);
-                }
+                $updateData['$set'][$mapping['name']] = $this->prepareReferencedDocumentValue($mapping, $new);
 
             // @ReferenceMany, @EmbedMany
             } elseif (

diff --git a/lib/Doctrine/ODM/MongoDB/Query/Builder.php b/lib/Doctrine/ODM/MongoDB/Query/Builder.php
@@ -1028,11 +1028,11 @@ public function nearSphere($x, $y = null): self
      * @see Expr::not()
      * @see https://docs.mongodb.com/manual/reference/operator/not/
      *
-     * @param array|Expr $expression
+     * @param array|Expr|mixed $valueOrExpression
      */
-    public function not($expression): self
+    public function not($valueOrExpression): self
     {
-        $this->expr->not($expression);
+        $this->expr->not($valueOrExpression);
 
         return $this;
     }

diff --git a/lib/Doctrine/ODM/MongoDB/Query/Expr.php b/lib/Doctrine/ODM/MongoDB/Query/Expr.php
@@ -861,11 +861,11 @@ public function nearSphere($x, $y = null): self
      * @see Builder::not()
      * @see https://docs.mongodb.com/manual/reference/operator/not/
      *
-     * @param array|Expr $expression
+     * @param array|Expr|mixed $valueOrExpression
      */
-    public function not($expression): self
+    public function not($valueOrExpression): self
     {
-        return $this->operator('$not', $expression);
+        return $this->operator('$not', $valueOrExpression);
     }
 
     /**

diff --git a/lib/Doctrine/ODM/MongoDB/UnitOfWork.php b/lib/Doctrine/ODM/MongoDB/UnitOfWork.php
@@ -1459,6 +1459,10 @@ public function scheduleForDelete(object $document, bool $isView = false): void
             unset($this->documentUpdates[$oid]);
         }
 
+        if (isset($this->documentUpserts[$oid])) {
+            unset($this->documentUpserts[$oid]);
+        }
+
         if (isset($this->documentDeletions[$oid])) {
             return;
         }

diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon
@@ -9,9 +9,8 @@ parameters:
         # Making classes final as suggested would be a BC-break
         -
             message: "#^Unsafe usage of new static\\(\\)\\.$#"
-            paths:
-                 - lib/Doctrine/ODM/MongoDB/Aggregation/Expr.php
-                 - lib/Doctrine/ODM/MongoDB/DocumentManager.php
+            count: 1
+            path: lib/Doctrine/ODM/MongoDB/Aggregation/Expr.php
 
         # This cannot be solved the way it is, see https://github.com/vimeo/psalm/issues/5788
         -
@@ -25,6 +24,24 @@ parameters:
             count: 1
             path: lib/Doctrine/ODM/MongoDB/Aggregation/Stage/GeoNear.php
 
+        # Fixed in 2.3
+        -
+            message: "#^Method Doctrine\\\\ODM\\\\MongoDB\\\\DocumentManager\\:\\:find\\(\\) should return T of object\\|null but returns object\\|null\\.$#"
+            count: 1
+            path: lib/Doctrine/ODM/MongoDB/DocumentManager.php
+
+        # Fixed in 2.3
+        -
+            message: "#^Return type \\(Doctrine\\\\ODM\\\\MongoDB\\\\Mapping\\\\ClassMetadataFactory\\) of method Doctrine\\\\ODM\\\\MongoDB\\\\DocumentManager\\:\\:getMetadataFactory\\(\\) should be compatible with return type \\(Doctrine\\\\Persistence\\\\Mapping\\\\ClassMetadataFactory\\<Doctrine\\\\Persistence\\\\Mapping\\\\ClassMetadata\\<object\\>\\>\\) of method Doctrine\\\\Persistence\\\\ObjectManager\\:\\:getMetadataFactory\\(\\)$#"
+            count: 1
+            path: lib/Doctrine/ODM/MongoDB/DocumentManager.php
+
+        # Making classes final as suggested would be a BC-break
+        -
+            message: "#^Unsafe usage of new static\\(\\)\\.$#"
+            count: 1
+            path: lib/Doctrine/ODM/MongoDB/DocumentManager.php
+
         # Union types cannot be added yet
         -
             message: "#^Result of && is always false\\.$#"

diff --git a/psalm-baseline.xml b/psalm-baseline.xml
@@ -0,0 +1,31 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<files psalm-version="4.7.3@38c452ae584467e939d55377aaf83b5a26f19dd1">
+  <file src="lib/Doctrine/ODM/MongoDB/DocumentManager.php">
+    <ParamNameMismatch occurrences="8">
+      <code>$document</code>
+      <code>$document</code>
+      <code>$document</code>
+      <code>$document</code>
+      <code>$document</code>
+      <code>$document</code>
+      <code>$documentName</code>
+      <code>$documentName</code>
+    </ParamNameMismatch>
+  </file>
+  <file src="lib/Doctrine/ODM/MongoDB/Mapping/ClassMetadata.php">
+    <ParamNameMismatch occurrences="2">
+      <code>$fieldName</code>
+      <code>$fieldName</code>
+    </ParamNameMismatch>
+  </file>
+  <file src="lib/Doctrine/ODM/MongoDB/PersistentCollection/PersistentCollectionTrait.php">
+    <ParamNameMismatch occurrences="1">
+      <code>$value</code>
+    </ParamNameMismatch>
+  </file>
+  <file src="lib/Doctrine/ODM/MongoDB/Query/QueryExpressionVisitor.php">
+    <ParamNameMismatch occurrences="1">
+      <code>$compositeExpr</code>
+    </ParamNameMismatch>
+  </file>
+</files>
diff --git a/psalm.xml b/psalm.xml
@@ -6,6 +6,7 @@
     xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
     xmlns="https://getpsalm.org/schema/config"
     xsi:schemaLocation="https://getpsalm.org/schema/config vendor/vimeo/psalm/config.xsd"
+    errorBaseline="psalm-baseline.xml"
 >
     <projectFiles>
         <directory name="lib/Doctrine/ODM/MongoDB" />

diff --git a/tests/Doctrine/ODM/MongoDB/Tests/Functional/EmbeddedTest.php b/tests/Doctrine/ODM/MongoDB/Tests/Functional/EmbeddedTest.php
@@ -431,6 +431,7 @@ public function testRemoveEmbeddedDocument()
 
         $check = $this->dm->getDocumentCollection(User::class)->findOne();
         $this->assertEmpty($check['phonenumbers']);
+        $this->assertNull($check['addressNullable']);
         $this->assertArrayNotHasKey('address', $check);
     }