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.

Sign up for GitHub

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

Make setCallback() add to previous callbacks #473

Closed
wants to merge 1 commit into from
Closed

Make setCallback() add to previous callbacks #473

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
10 changes: 9 additions & 1 deletion src/Definition.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,15 @@ public function getMaker()
*/
public function setCallback(callable $callback)
{
$this->callback = $callback;
if ($this->callback) { // "stack" the callbacks so that any previous callbacks will also be called.
$oldCallback = $this->callback;
$this->callback = function ($model, $saved) use ($oldCallback, $callback) {
// If the old callback returns false, the whole thing should return false.
return $oldCallback($model, $saved) !== false && $callback($model, $saved) !== false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Returning false to exit the callback chain is incompatible with the fact that false also determines if a save should go ahead or not. What if someone wants to exit the callback chain but still wants another persit to run?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand correctly, the current implementation will not persist the model if the callback returns false? So this line should make it so that if any of the callbacks return false, the model won't be persisted. Would you rather that all callbacks are called regardless of whether a prior one "failed" and then return false if any of them failed afterwards to prevent it being persisted?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand correctly, the current implementation will not persist the model if the callback returns false?

No, it will not persist the model again. Possibly someone might want the model to be updated in the database again after their callback does something, but does not want the remaining callbacks to be executed.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, okay.
So I should change it to something like

$this->callback = function ($model, $saved) use ($oldCallback, $callback) {
        $oldResult = $oldCallback($model, $saved);
        $newResult = $callback($model, $saved);
        return $oldResult !== false && $newResult !== false;
};

So that all the callbacks are always called, and returning false in any of them will prevent the second persist.

Then if we move this to addCallback(), then you can still use setCallback() to override all the other callbacks if you don't want them called.

};
} else {
$this->callback = $callback;
}

return $this;
}
Expand Down
46 changes: 46 additions & 0 deletions tests/DefinitionTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,40 @@ public function testCallbackDefinitionFunctions()
$this->assertNull($definition->getCallback());
}

public function testCallbacksStack()
{
static::$fm->define('CallbacksStackStub')->setCallback(function ($object, $saved) {
$object->field1 = true;
})->setCallback(function ($object, $saved) {
$object->field2 = true;
});

static::$fm->define('group:CallbacksStackStub')->setCallback(function ($object, $saved) {
$object->field3 = true;
});

$object = static::$fm->create('group:CallbacksStackStub');

$this->assertObjectHasAttribute('field1', $object);
$this->assertObjectHasAttribute('field2', $object);
$this->assertObjectHasAttribute('field3', $object);

// Test that stacked callback will return false if any of the callbacks return false.
$definition = static::$fm->getDefinition('CallbacksStackStub')->clearCallback();
$definition
->setCallback(function ($object, $model) { return false; })
->setCallback(function ($object, $model) { return true; })
->setCallback(function ($object, $model) { return true; });
$this->assertFalse(call_user_func($definition->getCallback(), null, null));

$definition = static::$fm->getDefinition('CallbacksStackStub')->clearCallback();
$definition
->setCallback(function () {})
->setCallback(function ($object) {})
->setCallback(function ($object, $model) { return true; });
$this->assertNotEquals(call_user_func($definition->getCallback(), null, null), false);
}

public function testDefineWithReplacementGenerators()
{
$user = static::$fm->create('UserModelStub', [
Expand Down Expand Up @@ -393,6 +427,18 @@ public function delete()
}
}

class CallbacksStackStub
{
public function save()
{
return true;
}

public function delete()
{
return true;
}
}
class UserModelStub
{
public function save()
Expand Down