About how Validation and Save work in CakePHP 3 ORM

[albertboada]

Hi there,

I've just started learning (and using) the new Cake3 ORM and I'm pretty confused about the way entities, save and validation work altogether.

Assume we have some validations set in UsersTable. Amongst them:

$validator->add('role', [
    'valid' => [
        'rule' => ['inList', ['admin', 'user', 'staff']],
        'message' => __('This is not a valid role'),
    ],
]);

If I execute the following code in UsersController:

$user = $this->Users->newEntity();
$user->role = 'meh';
$this->Users->save($user);
debug($user->errors());

, to my surprise, the entity WILL be saved and there will be no errors printed.

On the other hand, if we execute the following code:

$user = $this->Users->newEntity(['role' => 'meh']);
// or
// $user = $this->Users->newEntity();
// $user = $this->Users->patchEntity($user, ['role' => 'meh']);
$this->Users->save($user);
debug($user->errors());

, entity will NOT be saved (as it would be expected).

I know validation is executed in newEntity() or patchEntity() as a first measure. What shocks me is validation is not executed again in save(), making modified fields bypass validation. I also know there's this Request Validation vs Application Rules separation now, but there's not a lot of documentation about what you can and can't achieve with Application Rules (which leads to think this is not a case for Application Rules).

So, basically, what is the approach one should take in all this?

• It is weird and should be fixed?
• Repatch the $user Entity after modifying it (or be forced to repatch always instead of modifying), and then call save()? (the attractiveness of having built an ORM decreases a lot here)
• Build these validations in the Application Rules instead?

Thanks for your time!

[markstory]

It is weird and should be fixed?

No.

If your application is making changes to entities after they've been built, I would recommend using patchEntity() to do those changes if you need data type validations done. Application rules are used to ensure consistency before saving.

The thinking behind splitting validation and consistency checks is as follows. Generally unsafe data comes from the user, while internal application logic generally will not use incorrectly formatted/typed data. Therefore, validation is most useful when entities are created from user data. By validating data before it is converted into an entity, we can not copy invalid data into an entity and prevent many inconsistent states before a save is event attempted.

[nigeon]

Splitting validation, makes sense, but, from my point of view, it makes no sense not validating data passed to DB. I don't really care if the data was introduced by a user, or a developer...

An ORM that allows wrong data be in the DB... doesn't really sounds as it should be.

[albertboada]

So, basically, when we are defining validations in the model, we are actually defining user input validation now. I would understand this if the Application Rules feature offered the same range of built-in validations, but I think it doesn't (easily). A way to ensure every persisted Entity is fully consistent (wherever its data comes from) is not that crazy thing.

The way it's built now easily allows invalid entities to be persisted. Long-time Cake developers are not used to this, and shit is probably gonna happen.

For those interested in emulating the old Cake behaviour, you can maybe do this in your Table class:

EDIT: This patch below turned out not to be a good solution, since beforeSave will be never be executed if the entity already had errors. So, basically, setting entity properties after a "failed" patchEntity or newEntity would result in those properties not being validated anyway.

// Avoid. This is not a solution.
public function beforeSave($event) {
    $entity = $event->data['entity'];
    $this->patchEntity($entity, $entity->toArray());
    return !(bool)$entity->errors();
} 

(shittiest solution ever, I know). @markstory do you think this patch could have any side-efect (performance aside)? EDIT: as mentioned above, not a good solution.

[markstory]

@nigeon You can always use patchEntity() to modify entities, or use behaviors/subclasses to implement the re-validation.

It seems strange to me that the internals of an application would create invalidate states that would fail validation. For me the validation that is done in newEntity()/patchEntity() should be used to check data format and data types. Application rules should be used to do tasks like making sure email address are unique or that internal state machines are correct.

If there are gaps in the application rules features, I think we can look at ways to improve that once we've uncovered what those gaps are.

[nigeon]

@markstory yes, sure, it's just, feels weird when you build (as I do) first the data modeling layer, and after that, the rest of an app. I like the idea of "no matter what you do, the command for storing this to the DB will take care that nothing is wrong (validating, relations, data escaping....)".

"It seems strange to me that the internals of an application would create invalidate states that would fail validation."
Probably another topic, but, in the other side, PHP introduced type hinting with php5, and will add scalar type hinting support in php7, to prevent internal errors.

Anyway, thanks for replying.

[markstory]

@nigeon If you come across ways that the application rules could be improved I bet others would be interested in those improvements as well.

There are many other ORMs that don't provide validation or domain rules at all. Doctrine and SQLAlchemy are two that come to mind. Because of this we thought that our decisions to include both - albeit slightly differently than we have in the past - was an improvement.

[albertboada]

To me it feels weird anyway after all this time that Cake now doesn't naturally ensure all persisted Entities are valid, whatever methodology is used to set the data. And by naturally I mean not being kinda forced to use special methods like patchEntity() everytime we want to play around with what should be just simple objects. In fact, it's a regression of one of the most useful Cake features since the beginnings.

If Application Rules was powerful enough to be able to define the same range of validations that "normal" validation can, developers still willing to ensure consistency of persisted Entities would be able to as well. Both of 2 worlds, probably.

Will probably deal with it. Just wanted to make sure this behaviour was totally intended.

@markstory please, can you confirm that the way of patching this I mentioned above is not a super crazy thing to do?

Thanks!

[josegonzalez]

@albertboada Thats exactly what an entity is - it is a simple object. An entity is a bag of data that you happen to be able to add methods to. Since the validation rules are in the Table classes and the Entity classes themselves have no reference as to how to persist themselves or anything, it doesn't make sense to me that setting values in an entity would run validation rules.

[albertboada]

Not trying to make my point any more prominent, but I don't really get the point of @josegonzalez reply, so...

An Entity is a simple object. Yes, and as such, I would love to operate with it naturally.
save() is a Table method, and thus it should know about validation, as you indeed point out. Yes.

So the main point here is just how Cake developers would like to answer to this question:
Seeing this code below, do you think the save() should pass or should fail?

// UsersTable.php
$validator->add('email', 'valid', ['rule' => 'email']);

// UsersController.php
$user = new User;
$user->email = 'notAValidEmail';
$table->save($user);

---

it doesn't make sense to me that setting values in an entity would run validation rules.

No one said setting values in an Entity should run validation rules. But that certainly would solve lots of issues and would make everything more consistent, because what actually makes no sense is having 2 different ways of altering an Entity, both having completely different (and unguessable) consequences regarding validation when saving.

• patchEntity(): will make $entity->errors() be always up to date, and save() will indeed fail when it should fail.
• $entity->property=: will not make $entity->errors() be up to date, and save() will not fail when it should. Besides, there's no way to re-calculate errors on an Entity, is there?

When using simple object operations, save() is not aware of validation. In fact, it never really is.

So, basically, we are discouraging the use of simple object operations if we want everything to work as expected.

[markstory]

@albertboada How are the consequences of setting properties unguessable?

In your example above, I can see why that flow would seem incorrect, but outside of accepting user data, I don't think it is common for an application to set corrupted data. However, I think there is a opportunity to make it easier to define application rules that use the Validation class.

If it was possible to add rules that use Validation rules, we could move some of the datatype validation logic into rules and hopefully address some/most of this issue.

[htstudios]

I, too kike the idea of the model layer doing validations and using setter methods I could even like automatically run validations (even before save) and exceptions. I like the model lsyer to enforce correct data as soon and at any possible time. Especially when working with external (api, stored data) this makes sure there are no surprises.

[lorenzo]

Closing as the validation docs were improved to explain how it works

[xavier83ar]

I know that this issue is closed but I want to add some words to the discussion that maybe helps to rethink this subject.

One the things that I loved about cakephp 3 was the new ORM & Database Access, I understand and I do like the way application rules and validation rules were split.

But (yeah, there is a but) I think when those rules are checked is to be reviewed.
I would list some points I've been thinking on this matter:

1. It's a bit confused and not very intuitive to check validation rules as soon as data is assigned, and very often you would modify this data before you save it.
2. It's not the way it works on cakephp 2, I know that many things don't, but it's less intuitive for those that migrate.
3. Not always the data to build entities comes from forms, or from users data, it's very common to save related data that are build from other services (gis data, ip, ua, etc.), or data that needs to be processed before you save it.
   I think that, in this particular case, it's great to having the chance to work directly upon an entity, taking advantage of working with objects. Right now we should do it on an array, and then pass it to patchEntity() or newEntity() methods, because those only accept an array as new data (if I am right).
4. Many times we hear that a good programming practice is to having methods that do what they claim to do, neither patchEntity() nor newEntity() are very good in this aspect. It's not very easy to guess that they will validate the data, and already less intuitive that some data may get lost and not be there (on the entity).
5. Finally, we don't have a validate() method on Table Objects, to validate in a explicit way, like in cakephp 2.

So, the solution?

Beside thinking about the weak points of this approach, I been thinking on some possible solutions.

We already have an option for patchEntity and newEntity to disable validation, as have an option on save method to disabled application rule checking (all of them usefull for us to modify the validation workflow).
And Table Objects are responsible of all of them (patch, new, save), I think a solution could be adding an option 'validate' to save() method, with default value false. If it's true, then entity would be validated before attempting to check application rules or save if 'checkRules' => false.
As an alternative or in addition to, we could also have a property (and its respective method to handle it) on table objects to determine when validation occurs.

Something like...

// on some table object...

    /**
     * Initialize method
     *
     * @param array $config The configuration for the Table.
     * @return void
     */
    public function initialize(array $config)
    {
        $this->table('...');
        $this->validateOn('build'); //  possible values 'build' (default), 'save', 'both'
        ...
    }

This would allow as to setup this behavior on a per Table basis. Also default value should be 'build', to avoid breaking the API.

And finally it would be great to have an explicit validate() method on Table Objects, to validate when we want to.

[markstory]

And finally it would be great to have an explicit validate() method on Table Objects, to validate when we want to.

One challenge with this will be that all the validation methods would need to be compatible with pre and post casting representations of the data. While I think the core rules should be fine, I'm not quite as sure that application rules will always abide by that requirement.

Associations could also complicate a validate() method as well. It too would need to know which associations should be recursed into and have validation rules applied to them. Handling the association traversal is one of the reasons validation is done on entity build.

I think having a validate() method could be a nice addition to the Table class. Adding more modes and options to save() could muddle the ORM and make usage even more unclear to new and existing developers.

[albertboada]

👍

[elboletaire]

👍

[lorenzo]

I'm not convinced we should have such method. We are basically going to revive the reasons we had to come up with the current validation logic

[markstory]

We are basically going to revive the reasons we had to come up with the current validation logic.

@lorenzo What are the problems you see with a validate() method?

[lorenzo]

@markstory It depends, would it take an array or an entity?

[markstory]

I think the following is the ideal usage pattern

$article = $this->Articles->get(1);
$article->user_id = 'I am a mistake';

$result = $this->Articles->validate($article);
var_dump($result);
// false
var_dump($article->errors());
['user_id' => ['Must be a number']]

[ionas]

Would it check rules as well? If not, can rules be checked the same way?

[markstory]

Rules would be checked on save() as they do today.

[markstory]

Having rules checked in the same transaction window as save() can be important.

[ionas]

I do agree. I still don't know if wouldn't want to be able to "pre-check".
The example above makes a lot of sense. I am just getting into cake 3. How else would one do the thing above? patchEntity?

[lorenzo]

Closing as there is nothing to do here.

[elboletaire]

lol, serously? xD

[lorenzo]

Sorry for closing with a short explanation. The long one is that I'm confident that the current strategy of having stateless AND stageful validation as 2 separate steps was at the end the right solution. There is plenty documentation in the book for how to use both and how one can leverage the other. Moreover, it is possible to manually call any of those validation steps with arbitrary data.

If you have any specific suggesting for improvement, please open another ticket. This one got too long to track what's actually going on.

[elboletaire]

For stageful validation you mean the build rules?

[inoas]

Short: Yes.

Long: Stateful means that you are comparing the data you want to persist into the store with existing data.
E.g. if an email address already exists. If a domain is already registered on the network, etc.
Stateless validation never requires to know about the state of store that you want to save the data into. Stuff like email === email_repeat is an example.

Edit: I can see a point running both, stateless and stateful validations (rules) prior / independent of actually attempting to persist data. ->checkRules($entity) ->validate($entity) are method names I could see, if they don't exist yet.