callbacks and returning true

[euromark]

With the SoftDelete behavior you can spot a flaw in the current callback design.

If we - for example - want the behavior to fire its beforeDelete and "soft delete" a record we would like to return true to the controller, of course.

But we need to return false in order to stop the real delete from firing.

At least that is how it seems to be done right now - mainly due to this issue.

Can we introduce some model attribute like $continue etc which can be set to false from any callback to avoid

this?

So in the beforeDelete of the behavior:

    $model->continue = false; //default is true, of course

    return true;

We can now return true which the delete method then will, too, but without continuing any further. 

It can also reset itself to true for create() and other resetting methods.

[Ceeram]

I dont see this as a flaw in callbacks, although its called softdelete, it has nothing to do with delete, as its just setting a flag (update) on the record. When calling delete() when there is no actual delete query executed return value false would be expected.

i think doing if ($this->delete($id) || ($this->hasField('deleted') && $this->field('deleted', array($this->alias . 'id' => $id)))) {//deleted or softdeleted} would be a proper way to check if a field is deleted, you could also override delete() to run this check

SoftDeletable also doesnt take into account to softdelete cascading

Op zondag 20 mei 2012 11:24:48 UTC+2 schreef euromark het volgende:

[euromark]

I disagree that in this case it doesnt actually delete. for the controller (which uses the model) it does "look" like it actually deletes (due to the behavior attached). => black box principle

so the response needs to be true for the controller because otherwise the logic changes and cause unexpected results (in this case the error message instead of a success message).

Behaviors should be able to modify their models properly without creating overhead in the controller.

BUT

it is just an example of how a callback might want to return something despite the fact that this return value would affect the upcoming method chain.

therefore it might be a good idea to not make the return value decide if we abort or not but a model attribute we can set/alter.

[Cauan Cabral]

I think use a principle like Event Bubble is a best choice for this.

If you don't want event bubbling, only call a method like Event->stopPropagation()

Devs are familiar with propagation because other languages like javascript, actionscript or frameworks like .NET

--

Cauan Cabral
----------------

Como falar comigo: Google Talk: cau...@gmail.com Skype: CauanCabral MSN: cau...@gmail.com

Onde me encontrar:

[AD7six]

On Tuesday, 22 May 2012 12:46:47 UTC+2, Ceeram wrote:

I dont see this as a flaw in callbacks, although its called softdelete, it has nothing to do with delete, as its just setting a flag (update) on the record. When calling delete() when there is no actual delete query executed return value false would be expected.

i think doing if ($this->delete($id) || ($this->hasField('deleted') && $this->field('deleted', array($this->alias . 'id' => $id)))) {//deleted or softdeleted} would be a proper way to check if a field is deleted, you could also override delete() to run this check

SoftDeletable also doesnt take into account to softdelete cascading

I think caching is a better example - It's not possible to transparently attach a behaviour to hijack certain queries and read from the cache bailing early; because you can only either let the/a query continue, or abort the current call and return false.

All in all I understand the desire to permit callbacks to do more than modify or abort, and think it's worth thinking about.

AD

[mark_story]

Adding caching via a behavior is a very different situation I think.  Generally in that situation you want to abort the find operation and just pipe back results.  Currently both queries and results are arrays so inferring which one a behavior has returned is not possible.  However, imagine a place where callbacks are controller by events, and both query data and result data are object types.

function beforeFind($event ....) {

   $event->stopPropagation(); // cancel

   // talk to cache

   $event->result  = $cacheData;

}

If the code handling events knew to check the result property for find results, you could both stop the event and attach the data that should be returnned.  While we can't convert everything over to the event system now, as it breaks BC hard.  I'm also strongly opposed to more model properties with more conditional and edge case usage.  We should be trying to make a cleaner way to do things not adding more bolts and sheet metal into the frankenstein monster that is the model api.

I think by working toward the event system, for all callbacks we can accomodate situations like the soft delete and caching finds through a behavior.

-Mark

[euromark]

sounds good to me.

so something to think about for 3.0 then when model rework is done anyway.

[Jamie Nay]

I spent a lot of time looking for a better solution too, but couldn't
come up with something good. So my solution is similar to Augusto's in
that I override Model::delete to check for soft-deleted models (models
with a deleted = true column value):

public function delete($id = null, $cascade = true) {
$deleted = parent::delete($id, $cascade);
return $deleted || ($this->hasField('deleted') &&
$this->findByIdAndDeleted($id, true));
}

On 2013-03-13 3:27 AM, David Yell wrote:
> Is there an acceptable way to organise this in 2.x? I'd really rather
> not look for a false in my controller to feedback to the user. I guess
> using Augusto's method will be okay as a stopgap. --
> You received this message because you are subscribed to the Google
> Groups "cakephp-core" group.
> To unsubscribe from this group and stop receiving emails from it, send
> an email to cakephp-core...@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
>
>