Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix segfault on Model::save() with circular relations #13354

Merged
merged 1 commit into from
May 24, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Fix segfault on infinite recursion into circular model relations
In cases where related models were set before save on *both* sides of a
hasMany/belongsTo relationship, `Model::save()` would go into an infinite
loop as its pre and post-save checks for relations to save never checked
if the related model had already been saved:

    Save A
     \_ A hasMany B
      \_ Save B
       \_ B belongsTo A
        \_ Save A
         \_ ...

For me this was causing a segfault, though with xdebug installed it would
error before crashing at the max_nesting_level.
  • Loading branch information
stecman committed May 24, 2019
commit 62136f35a0996cc1358a5ae22e6070ba99043fb9
1 change: 1 addition & 0 deletions CHANGELOG-4.0.md
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
- Fixed `method` parameter in `Phalcon\Mvc\Model\Manager::getHasOneRecords()`, it's not always a string, null by default. [#14115](https://github.com/phalcon/cphalcon/issues/14115)
- Fixed `method` parameter in `Phalcon\Mvc\Model\Manager::getHasManyRecords()`, it's not always a string, null by default. [#14115](https://github.com/phalcon/cphalcon/issues/14115)
- `handlers` property in `Phalcon\Mvc\Micro\Collection` is now always an array.
- Fixed crash in `Phalcon\Mvc\Model::save()` when saving a circular model relation. [#13354](https://github.com/phalcon/cphalcon/pull/13354)

## Removed
- Removed `Phalcon\Session\Factory`. [#13672](https://github.com/phalcon/cphalcon/issues/13672)
Expand Down
6 changes: 3 additions & 3 deletions phalcon/Mvc/Model.zep
Original file line number Diff line number Diff line change
Expand Up @@ -4625,10 +4625,10 @@ abstract class Model implements EntityInterface, ModelInterface, ResultInterface
}

/**
* If dynamic update is enabled, saving the record must not
* take any action
* 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->save() {
if record->dirtyState !== Model::DIRTY_STATE_PERSISTENT && !record->save() {
/**
* Get the validation messages generated by the
* referenced model
Expand Down
27 changes: 27 additions & 0 deletions tests/integration/Mvc/Model/SaveCest.php
Original file line number Diff line number Diff line change
Expand Up @@ -334,4 +334,31 @@ public function mvcModelSaveAfterWithoutDefaultValues(IntegrationTester $I)
$robot->delete()
);
}

public function mvcModelSaveCircularRelation(IntegrationTester $I)
{
$I->wantToTest('Mvc\Model::save() with circular unsaved relations');

$album = new Albums([
'name' => 'Loopback'
]);
$artist = new Artists([
'name' => 'Evil Robot'
]);

// Assign relationship in both directions on unsaved models
$album->artist = $artist;
$artist->albums = [
$album
];

// Save should handle the circular relation without issue
$I->assertTrue(
$artist->save()
);

// Both should have an ID now
$I->assertNotEquals(null, $album->id);
$I->assertNotEquals(null, $artist->id);
}
}