Fixed infinite save loop phalcon#16395

CHANGELOG-5.0.md

@@ -16,6 +16,7 @@
 ### Fixed
 
 - Parse multipart/form-data from PUT request [#16271](https://github.com/phalcon/cphalcon/issues/16271)
+- Infinite save loop in Model::save() [#16395](https://github.com/phalcon/cphalcon/issues/16395)
 
 
 ## [5.2.3](https://github.com/phalcon/cphalcon/releases/tag/v5.2.3) (2023-07-26)

phalcon/Mvc/Model.zep

@@ -42,6 +42,7 @@ use Phalcon\Mvc\Model\ValidationFailed;
 use Phalcon\Mvc\ModelInterface;
 use Phalcon\Filter\Validation\ValidationInterface;
 use Serializable;
+use SplObjectStorage;
 
 /**
  * Phalcon\Mvc\Model
@@ -2579,12 +2580,34 @@ abstract class Model extends AbstractInjectionAware implements EntityInterface,
      * $robot->save();
      *```
      */
+
     public function save() -> bool
+    {
+        var visited;
+        let visited = new SplObjectStorage();
+        return this->doSave(visited);
+    }
+
+    /**
+     * Inserted or updates model instance, expects a visited list of objects.
+     *
+     * @param SplObjectStorage $visited
+     *
+     * @return bool
+     */
+
+    public function doSave(<SplObjectStorage> visited) -> bool
     {
         var metaData, schema, writeConnection, readConnection, source, table,
             identityField, exists, success, relatedToSave;
         bool hasRelatedToSave;
 
+        if true === visited->contains(this) {
+            return true;
+        }
+
+        visited->attach(this);
+
         let metaData = this->getModelsMetaData();
 
         /**
@@ -2610,7 +2633,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 +2718,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 +2734,8 @@ abstract class Model extends AbstractInjectionAware implements EntityInterface,
                  */
                 let success = this->postSaveRelatedRecords(
                     writeConnection,
-                    relatedToSave
+                    relatedToSave,
+                    visited
                 );
             }
         }
@@ -4964,9 +4988,10 @@ abstract class Model extends AbstractInjectionAware implements EntityInterface,
      * Saves related records that must be stored prior to save the master record
      *
      * @param ModelInterface[] related
+     * @param SplObjectStorage visited
      * @return bool
      */
-    protected function preSaveRelatedRecords(<AdapterInterface> connection, related) -> bool
+     protected function preSaveRelatedRecords(<AdapterInterface> connection, related, <SplObjectStorage> visited) -> bool
     {
         var className, manager, type, relation, columns, referencedFields, nesting, name, record;
 
@@ -4981,6 +5006,7 @@ abstract class Model extends AbstractInjectionAware implements EntityInterface,
             manager = <ManagerInterface> this->getModelsManager();
 
         for name, record in related {
+
             /**
              * Try to get a relation with the same name
              */
@@ -5006,7 +5032,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 +5048,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
@@ -5074,7 +5099,7 @@ abstract class Model extends AbstractInjectionAware implements EntityInterface,
      * @param ModelInterface[] related
      * @return bool
      */
-    protected function postSaveRelatedRecords(<AdapterInterface> connection, related) -> bool
+     protected function postSaveRelatedRecords(<AdapterInterface> connection, related, <SplObjectStorage> visited) -> bool
     {
         var nesting, className, manager, relation, name, record,
             columns, referencedModel, referencedFields, relatedRecords, value,
@@ -5157,7 +5182,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 +5246,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 +5269,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 <team@phalcon.io>
+     * @since  2023-08-09
+     * @issue  #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[][]
      */