cakephp: RFC 3.4: Allow strict save() call that throws exception.

This is a (multiple allowed):

bug
enhancement
feature-discussion (RFC)

What you did

This has come up a lot in the past, and I also happen to have been blind-sighted by it:

// void method
...
$this->ModelName->save($entity);

Now one would expect this to save, and it probably does. But at any point in the future with modified validation and rules this might not. And it will give no indication and just silently do nothing.

Current fix

This is sure the OK case for validation inside forms. But if you have non-validation code, e.g. in CLI calls, in important business logic etc, you definitly do not want those silent no-ops.

So that is probably what most do, they wrap it everywhere:

// void method
if (!$this->ModelName->save($entity)) {
    throw new \Exception('Could not save ModelName: ' . print_r($entity->errors(), true));
}

So that at least in those cases where failing would at no point be acceptable it will notify the developer and user early on.

Core solution?

So this can get a bit annoying to repeat all over again, can this maybe be wrapped as core functionality? I bet many others would also prefer sth like:

// void method
...
$this->ModelName->save($entity, ['strict' => true]);

We could also use 'exception' => true/false (default false for BC) or some other speakable word. Feedback?

//EDIT Same issue for delete and the case of rules going bad:

if ($options['checkRules'] && !$this->checkRules($entity, RulesChecker::DELETE, $options)) {
    return false;
}

Can lead to a silent “It looks like it deleted something, actually nothing changed though” scenario.

About this issue

Original URL
State: closed
Created 8 years ago
Reactions: 5
Comments: 34 (34 by maintainers)

Most upvoted comments

@dereuromark wouldn’t a callback like failedSave() and failedDelete() be an option for you? Cause it seems to me to be a generally useful thing…

ravage84 on Nov 14, 2016

Good work!

inoas on Feb 17, 2017

Thanks a lot!

dereuromark on Feb 16, 2017

Can I give this a try? Implementing failedSave() and failedDelete()

cleptric on Jan 25, 2017

I am fine either way as long as there aren’t any hidden “didnt work” timebombs in the code where you don’t respond with validation to the user, but silently just return afterwards. Until then this for now works quite well as interim solution for save() and delete().

dereuromark on Feb 1, 2017

I am 100% with @thinkingmedia - people might have to do some actual project (business critical) coding to know the issues involved here. Seems like @ravage84 might not have yet, otherwise he would understand that “misuse” is absolutely the wrong conclusion drawn, as well as “data validation” not part of any of the above topic. It is about sane and safe database operations outside of the normal validation cycle of CRUD - as I clearly pointed out before.

Again: none of those exceptions are supposed to be caught or EVER happen in production systems (otherwise you would use normal validation and handle it!). If they do then for a very faulty system - and that needs to be caught early and always, immediately addressed, and never cloaked (or silently ignored).

dereuromark on Nov 9, 2016

Here we do need the exceptions, but we even more need them where you can’t feasibly do returns and check everywhere for them

I don’t buy that argument. There are entire languages with no exceptions, and people are pretty productive with those tool chains. I worry that adding more ‘modes’ into already complex code is going to exacerbate the issue.

markstory on Oct 26, 2016