CareSet / DURC

DURC is reverse CRUD
MIT License
3 stars 2 forks source link

1.0 validation functions #61

Closed ftrotter closed 4 years ago

ftrotter commented 4 years ago

Very happy to have Ken demonstrating the depth that using the Laravel Illuminate Validation rules give us. I would like to implement the following validation rules out of the box... I really want to lean into this, since I think it will make lots of data import processes at the company far more effective. Here are the validation functions that I would like to implement out of the gate.

This seems reasonable given what is simple to implement from: https://laravel.com/docs/6.x/validation#available-validation-rules

ftrotter commented 4 years ago

I am realizing that we may need to have something that will accept a zero, but not a blank... Which is between 'required' and 'present' in the validatin scheme.

And that our desire to have something that make sense with our HTML form environment might be at odds with our desire to use this for low level data processing...

-FT

kchapple commented 4 years ago

@ftrotter @seanccsmith

One thing that may be throwing us off is a default-enabled middleware that automatically converts empty strings to null. This converts any empty string that comes from the UI to a null before the Request object reaches the controller.

You can see in app/Http/Kernel.php that the following middleware is listed. \Illuminate\Foundation\Http\Middleware\ConvertEmptyStringsToNull::class

Since this middleware is enabled by default, it's not possible to have values be 'present' as you would like. Instead, an Exception is thrown when an empty string (then converted to null) is attempted to be saved into the database. To me, you want to potentially allow these three cases:

  1. If the field can be empty, the default value can be be set to an empty-string by default
  2. If there's no default value, and the field cannot be NULL, the user can still input an empty-string
  3. If there's no default value, and the field cannot be NULL, and the there is the string 'required' in the name, then the field should be explicitly required

I don't know if this is right (allowing point 2) for most cases, as most of the time, if you don't have a default value, and the field cannot be null, the application would expect some value. Like for a 'name' field for example. Should the user be able to enter an empty-string for their name in a real application?

I think that if there is no default value, and the field cannot be null, that the field should be required by the application. That will also ease confusion around the middleware described above.

Let me know if you have different experiences with your use-cases, as I understand that this firstly needs to support your apps.

kchapple commented 4 years ago

I changed present back to required and documented. 'integer' validation allows 0 values. empty strings are converted to nulls by Laravel, so we changed those to required cause that seems to make sense for 99.9% of use-cases.

seanccsmith commented 4 years ago

Closed the wrong issue

seanccsmith commented 4 years ago

*_url fields do not seem to be validated: bad strings entered through the web report interface are placed into the database.

seanccsmith commented 4 years ago

Update:

seanccsmith commented 4 years ago

int/numeric floors and ceilings will be difficult to implement, and look unlikely to actually limit input the way we want just based on the data type. This will be moved to a 1.1 issue.

seanccsmith commented 4 years ago

I verified that I have the updated code, but I'm still getting bad results. The uri validator seems not to be working at all, and the boolean validator is weird too, but I'm not sure if it's the validators (screenshot).

Screen Shot 2020-06-05 at 1 47 12 PM

The original test datum for the is_boolean field is 4321. If I'm understanding this correctly, it looks like that is being evaluated to 1 BEFORE it is passed to the validator - is that true? Or is this a validator problem somehow?

kchapple commented 4 years ago

@seanccsmith Yes, the boolean thing is something @ftrotter added:

    public static function formatForStorage( $field_name, $field_type, $value, $model = null)
    {

        $formattedValue = $value;
        if ( self::mapColumnDataTypeToInputType( $field_type, $field_name ) == 'boolean' ) {
            //support obvious notions of truth
            if ( $value === 'on' || $value === 'true' || $value === true || $value > 0 ) {
        //this allows us to support the use of 'on'/'true' etc  for trueness
                $formattedValue = 1;
            } else {
        //if it is a boolean and it is not obviously true.. then it is false..
                $formattedValue = 0;
            }
kchapple commented 4 years ago

@seanccsmith can you verify if you 'cd vendor/careset/durc' and then do 'git log --oneline' that this is the output:

199a159 (HEAD -> master, tag: v0.9, origin/master, origin/HEAD) Added _uri suffix to trigger url validation rule, added is_ and has_ prefix to trigger boolean validation rule (#61)
seanccsmith commented 4 years ago

Hmm, I don't know about this. So if we have a situation where the user has completely misunderstood the purpose of the column, by putting "4321" or "mostly harmless" in a field that should be boolean, we're making a judgement on it instead of throwing an error?

There's no way to get to the validator here, if any input will be evaluated to 0 or 1.

seanccsmith commented 4 years ago

@kchapple Yes that's the output on my server when I run that command

seanccsmith commented 4 years ago

Everything here is now functioning except for the boolean validators, which are being subverted by that boolean converter. I will make a new issue for that with a v1.1 flag.