Fixed infinite save loop #16395

CHANGELOG-5.0.md

@@ -1,5 +1,11 @@
 # Changelog
 
+## [5.3.1](https://github.com/phalcon/cphalcon/releases/tag/v5.3.1) (xxxx-xx-xx)
+
+### Fixed
+- Infinite save loop in Model::save() [#16395](https://github.com/phalcon/cphalcon/issues/16395)
+
+
 ## [5.3.0](https://github.com/phalcon/cphalcon/releases/tag/v5.3.0) (2023-08-15)
 
 ### Added

phalcon/Mvc/Model.zep

@@ -41,6 +41,8 @@ use Phalcon\Mvc\Model\TransactionInterface;
 use Phalcon\Mvc\Model\ValidationFailed;
 use Phalcon\Mvc\ModelInterface;
 use Phalcon\Filter\Validation\ValidationInterface;
+use Phalcon\Support\Collection;
+use Phalcon\Support\Collection\CollectionInterface;
 use Serializable;
 
 /**
@@ -2579,12 +2581,35 @@ abstract class Model extends AbstractInjectionAware implements EntityInterface,
      * $robot->save();
      *```
      */
+
     public function save() -> bool
+    {
+        var visited;
+        let visited = new Collection();
+        return this->doSave(visited);
+    }
+
+    /**
+     * Inserted or updates model instance, expects a visited list of objects.
+     *
+     * @param CollectionInterface $visited
+     *
+     * @return bool
+     */
+    public function doSave(<CollectionInterface> visited) -> bool
     {
         var metaData, schema, writeConnection, readConnection, source, table,
-            identityField, exists, success, relatedToSave;
+            identityField, exists, success, relatedToSave, objId;
         bool hasRelatedToSave;
 
+        let objId = spl_object_id(this);
+
+        if true === visited->has(objId) {
+            return true;
+        }
+
+        visited->set(objId, this);
+
         let metaData = this->getModelsMetaData();
 
         /**
@@ -2610,7 +2635,7 @@ abstract class Model extends AbstractInjectionAware implements EntityInterface,
         let hasRelatedToSave = count(relatedToSave) > 0;
 
         if hasRelatedToSave {
-            if this->preSaveRelatedRecords(writeConnection, relatedToSave) === false {
+            if this->preSaveRelatedRecords(writeConnection, relatedToSave, visited) === false {
                 return false;
             }
         }
@@ -2695,7 +2720,7 @@ abstract class Model extends AbstractInjectionAware implements EntityInterface,
         /**
          * Change the dirty state to persistent
          */
-        if success {
+        if true === success {
             let this->dirtyState = self::DIRTY_STATE_PERSISTENT;
         }
 
@@ -2711,7 +2736,8 @@ abstract class Model extends AbstractInjectionAware implements EntityInterface,
                  */
                 let success = this->postSaveRelatedRecords(
                     writeConnection,
-                    relatedToSave
+                    relatedToSave,
+                    visited
                 );
             }
         }
@@ -4964,9 +4990,11 @@ abstract class Model extends AbstractInjectionAware implements EntityInterface,
      * Saves related records that must be stored prior to save the master record
      *
      * @param ModelInterface[] related
+     * @param CollectionInterface visited
      * @return bool
      */
-    protected function preSaveRelatedRecords(<AdapterInterface> connection, related) -> bool
+
+    protected function preSaveRelatedRecords(<AdapterInterface> connection, related, <CollectionInterface> visited) -> bool
     {
         var className, manager, type, relation, columns, referencedFields, nesting, name, record;
 
@@ -5006,7 +5034,6 @@ abstract class Model extends AbstractInjectionAware implements EntityInterface,
                             "Only objects can be stored as part of belongs-to relations in '" . get_class(this) . "' Relation " . name
                         );
                     }
-
                     let columns = relation->getFields(),
                         referencedFields = relation->getReferencedFields();
 //                    let columns = relation->getFields(),
@@ -5023,7 +5050,7 @@ abstract class Model extends AbstractInjectionAware implements EntityInterface,
                      * If dynamic update is enabled, saving the record must not take any action
                      * Only save if the model is dirty to prevent circular relations causing an infinite loop
                      */
-                    if record->dirtyState !== Model::DIRTY_STATE_PERSISTENT && !record->save() {
+                    if record->dirtyState !== Model::DIRTY_STATE_PERSISTENT && !record->doSave(visited) {
                         /**
                          * Get the validation messages generated by the
                          * referenced model
@@ -5072,9 +5099,10 @@ abstract class Model extends AbstractInjectionAware implements EntityInterface,
      * Save the related records assigned in the has-one/has-many relations
      *
      * @param ModelInterface[] related
+     * @param CollectionInterface visited
      * @return bool
      */
-    protected function postSaveRelatedRecords(<AdapterInterface> connection, related) -> bool
+    protected function postSaveRelatedRecords(<AdapterInterface> connection, related, <CollectionInterface> visited) -> bool
     {
         var nesting, className, manager, relation, name, record,
             columns, referencedModel, referencedFields, relatedRecords, value,
@@ -5157,7 +5185,7 @@ abstract class Model extends AbstractInjectionAware implements EntityInterface,
                         /**
                          * Save the record and get messages
                          */
-                        if !recordAfter->save() {
+                        if !recordAfter->doSave(visited) {
                             /**
                              * Get the validation messages generated by the
                              * referenced model
@@ -5221,7 +5249,7 @@ abstract class Model extends AbstractInjectionAware implements EntityInterface,
                         /**
                          * Save the record and get messages
                          */
-                        if !intermediateModel->save() {
+                        if !intermediateModel->doSave(visited) {
                             /**
                              * Get the validation messages generated by the referenced model
                              */
@@ -5244,7 +5272,7 @@ abstract class Model extends AbstractInjectionAware implements EntityInterface,
                         /**
                          * Save the record and get messages
                          */
-                        if !recordAfter->save() {
+                        if !recordAfter->doSave(visited) {
                             /**
                              * Get the validation messages generated by the
                              * referenced model

tests/database/Mvc/Model/SaveCest.php

@@ -644,6 +644,35 @@ public function mvcModelSaveWithRelatedManyAndBelongsRecordsProperty(DatabaseTes
         $I->assertEquals($expected, $actual);
     }
 
+    /**
+     * Tests Phalcon\Mvc\Model\ :: save() Infinite Loop
+     *
+     * @author Phalcon Team <[email protected]>
+     * @since  2023-08-09
+     * @issue  https://github.com/phalcon/cphalcon/issues/16395
+     *
+     * @group  mysql
+     * @group  sqlite
+     */
+    public function infiniteSaveLoop(DatabaseTester $I)
+    {
+        $I->wantToTest('Mvc\Model - save() infinite Save loop');
+
+        /** @var \PDO $connection */
+        $connection = $I->getConnection();
+        $invoicesMigration = new InvoicesMigration($connection);
+        $invoicesMigration->insert(77, 1, 0, uniqid('inv-', true));
+
+        $customersMigration = new CustomersMigration($connection);
+        $customersMigration->insert(1, 1, 'test_firstName_1', 'test_lastName_1');
+
+        $customer = Customers::findFirst(1);
+        $invoice = Invoices::findFirst(77);
+        $invoice->customer = $customer;
+        $customer->invoices = [$invoice];
+        $customer->save();
+    }
+
     /**
      * @return \string[][]
      */