spadefoot / kohana-orm-leap

An ORM module for the Kohana PHP framework that is designed to work with all major databases.
http://spadefoot.github.io/kohana-orm-leap/
100 stars 25 forks source link

Model validation #51

Open taai opened 11 years ago

taai commented 11 years ago

Is there a Model validation built in? I don't see it.

Will it get implemented? If not, how I'm supposed to validate models?

taai commented 11 years ago

Alright...

Possible solutions

Create a static function rules() wich returns an array with rules for Validation class. And build in a $validation->check() in the save() function. There should be the second argument $extra_validation to that function.

What do you think?

taai commented 11 years ago

Thoughts about possible implementations

<?php
try {
    $model->save();
}
catch (Leap_ORM_Validation $e) {
    $errors = $e->errors('car');
}
<?php
try {
    $model
        ->validate() // <--- here!
        ->save();
}
catch (Leap_ORM_Validation $e) {
    $errors = $e->errors('car');
}
<?php
if ($errors = $model->validate()) {
    return;
}

$model->save();

I think that combo of first and second could be the best way. Please, tell me your thoughts about this!

bluesnowman commented 11 years ago

Honestly, I am not a big fan of the way Kohana handles validation. This doesn't mean that I am oppose to it completely, I just think that it introduces a number of limitations. This is something we have discussed internally and so give me a day to formulate our thoughts and enumerate them for you. I will get back to you shortly.

taai commented 11 years ago

I'm not going to reinvent Validation. I just want to integrate the Kohana's Validation in Leap, just like in the Kohana ORM. :) Oups! I did misread. So, you are not big fan of Kohana's validation.

taai commented 11 years ago

@bluesnowman , please comment this issue!

Current Kohana_BadData_Exception pisses me off. :D For example, Datetime field is not accepting timestamps with timezone.

Why Kohana's Validation is not good, what limitations it introduces? What do you suggest instead?

taai commented 11 years ago

By the way, I also can't get the actual error from Kohana_BadData_Exception, because it wont tell me was the string too long, or the regular expression did not match, or the string was not found in enum list... Any way I have to use a validation module!

taai commented 11 years ago

Still no decision about this?

bluesnowman commented 11 years ago

@taai Do you think it possible to add Kohana's Validation without making Leap completely dependent on the Kohana framework? The reason I ask is because I would love to try to make Leap usable by other frameworks while keeping it primarily for Kohana.

jsand commented 11 years ago

@bluesnowman Yes, it's possible. I only quickly browsed the Validation classes (system/classes/kohana/validation.php and system/classes/kohana/valid.php) and there are a few places where you would have to remove Kohana-dependent code (Profiler, Kohana_Exception, Kohana::message, Kohana::find_file). The UTF8 library would also have to be brought into the mix.

bluesnowman commented 11 years ago

@taai Yeah, those are definitely things that would need to change. If we can do it, I would definitely be for it.

taai commented 11 years ago

@bluesnowman But we depend on Kohana already. For example, the exceptions are based on Kohana_Exception and I wouldn't be surprised, if there are other dependencies on Kohana.

Also if we are including everything in the Leap, it would make Leap fat. In my opinion it's better to use classes from existing environment - Kohana.

There is a great programming tip that I myself desperately trying stick to: "Make code usable before making it reusable". The features are needed right now and I think that Leap can be made "universal" later. Currently the title says "LEAP ORM for Kohana PHP Framework", so I didn't even think about the Leap as completely standalone module.

Possible solution - don't make built-in Validation, if the goal is to make Leap universal, but instead make callback functions, that provides argument with all the Model values. In that way programmers can choose what validation class to use, if any at all. For example:

<?
class Model_Leap_Car extends DB_ORM_Model {

    public static $validation_callback = array('Validation_Callback_Class', 'validate');

}

And then when the model will get saved/created/updated, this callback will get called.

bluesnowman commented 11 years ago

@taai Yes, you are correct that LEAP does depend on Kohana. I am just advocating that we minimize the number of Kohana dependencies so that it is possible later to open LEAP to other frameworks.

In reference to your example, we did discuss about a month ago about changing the Kohana_Exception dependencies to use a Leap_Exception instead, which will not make the ORM fat, but it does make it less dependent on Kohana which is what I would like to see as Leap matures.

I agree that we do not want LEAP to become fat; I also prefer a lean architecture. My suggestion to minimize dependencies should not be construed that I want a fat architecture but just that I want to encourage design changes that make sense and take into account that dependencies should not be unnecessarily interjected. I agree that it is wise to make code usable before reusable, but I also adhere to a philosophy that injecting unnecessary dependencies often leads to major problems and major code refactoring later.

Please do not interpret that I am saying your ideas are unnecessary...I am just saying that I want that we limit dependencies when possible and find solution that allows people like myself to use another form of validation should they not like Kohana's method. I just want to make sure LEAP stays as flexible as it can be.

I like your callback suggestion. I like such improvements. Your suggestion is a good one. I like it because it still maintains the decoupling that I want to see in LEAP.

However, I think there is some confusion about the purpose of the field classes. These classes are not meant to be validation classes; they are meant to enforce strong data typing that PHP lacks and some databases lack (e.g. SQLite). Although I have built some validation rules into these classes, that is not their main purpose. They are really meant for data integrity purposes, not validation.

taai commented 11 years ago

@bluesnowman All that makes perfect sense. Now we have to build something. I need your leadership so I know what to do and what are the goals and future structure. :)

I understand about the data integrity checking and I'm for it. It's just not that good yet. And people probably are expecting it to be the validation, but now we know that it's not. Two things that I would like to change:

  1. Make Field regular expressions customizable (not harcoded), because currently I have to make a custom Field class, because the PostgreSQL timestamp contains also miliseconds...
  2. When reading data from database, there should not be so much of validation / data integrity checking. Probably there isn't, but it just feels unnecesarry.

The validation is needed anyway, because there are not just integrity validation, but also conditional validation - if value A is empty, the value B must be empty too, and such. Model-related things, especially validation, must be done inside the Model, not Controllers, and this is what now I'm waiting for. :)