Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[2.1.x] Uniqueness validator should convert values. #12005

Closed
basilfx opened this issue Jul 22, 2016 · 7 comments
Closed

[2.1.x] Uniqueness validator should convert values. #12005

basilfx opened this issue Jul 22, 2016 · 7 comments

Comments

@basilfx
Copy link
Contributor

basilfx commented Jul 22, 2016

The Uniqueness validator executes a query on the database to see if it unique. But it uses the values from the model instance as-is. In my case, I convert high-level object to database-compatible values. For instance, I like to use DateTime, store 'structures' in JSON, and so on.

In the example below, the Uniqueness validator builds a query using $this->deleted, which is still a DateTime object because beforeSave is not yet invoked. This makes the query invalid, because PHP cannot convert a DateTime object into a string.

The simple fix would be to call beforeSave() before all of the validations run. This would ensure that a query is build using the values that would otherwise be submitted to the database. But I don't want that, because it could break other validators (e.g. ones that DO expect richer objects). After all, the validation classes are generic and not explicitly bound to the ORM. I could use them for non-ORM things.

Example model:

class User extends Model {
    private $name;
    private $deleted;

    private function beforeSave()
    {
        if ($this->deleted !== null) {
            $this->deleted = $this->deleted->format("Y-m-d H:i:s");
        }
    }
    private function afterFetch()
    {
        if ($this->deleted !== null) {
            $this->deleted = new \DateTime($this->deleted);
        }
    }

    public function validation()
    {
        $validator = new Validation();
        $validator->add(
            ["name", "deleted"],
            new Uniqueness([
                "message" => "The name must be unique.",
            ])
        );

        return $this->validate($validator);
    }
}
@Jurigag
Copy link
Contributor

Jurigag commented Jul 22, 2016

Wait a minute ! Isn't that why we have beforeValidationOnSave ? Just change your beforeSave to beforeValidationOnSave and it should work without workaround.

@basilfx
Copy link
Contributor Author

basilfx commented Jul 22, 2016

That would not work if I want to have a custom validator that uses a DateTime while also use Uniqueness that requires a timestamp as string.

@Jurigag
Copy link
Contributor

Jurigag commented Jul 22, 2016

Then just in this validator create datetime. Why beforeSave should be called if there is already beofreValidationOnSave ? Then both would to the same - call some function before save and validation.

Also i don't understand this issue - you are telling that fix can be to call beforeSave(which is already done, it's just called beforeValidationOnSave) but it's not a fix because you wan't objects in validators, not strings.

If you need BOTH string and datetime object then create as fields in class or something.

Also how uniqueness validator is supposed to know how you want to convert those values to what ?

Isn't getting all fields from metadata, checking their type if they date/datetime, converting datetime object to string just for uniqueness validation seriously break for SOLID ?

@basilfx
Copy link
Contributor Author

basilfx commented Jul 22, 2016

I just found out that my fix wasn't actually working. Guess I had some source file not saved and thought it was working. I removed it from the issue.

Let me rephrase completely, using an example from this documentation page.

<?php

class Robots extends Model
{
    public $id;

    public $name;

    public $status;

    public function beforeSave()
    {
        // Convert the array into a string
        $this->status = join(',', $this->status);
    }

    public function afterFetch()
    {
        // Convert the string to an array
        $this->status = explode(',', $this->status);
    }

    public function afterSave()
    {
        // Convert the string to an array
        $this->status = explode(',', $this->status);
    }
}

So if I create $robot = new Robot(), I can safely do $robot->status = [1, 2, 3, 4] and it will be converted to a string before it is saved to the database. The reverse applies when I fetch it from the database.

Now I want to do two validations (just something I made up):

  1. $robot->status should be an array and contain 2 and 3.
  2. $robot->status must be an unique set of numbers in the database.

For (1) I would write my own custom validator that accepts an array. I write this validator in a generic way, because I could also use it to validate a form or some other random value. In other words, the contract for this validator is that the input is always an array.

For (2) I would like to use the Validator\Uniqueness.

Now I add both validators to to a validator, implement the validate() method and start validating. This is where it fails: (1) requires the value as-is while (2) requires the value converted, even if I would convert it using beforeValidationOnSave.

If you need BOTH string and datetime object then create as fields in class or something.

According to the example, this is how I should implement 'both fields'.

Also how uniqueness validator is supposed to know how you want to convert those values to what?

That's the problem I am looking a valid and generic solution for. I think the current implementation is limited for this problem.

Isn't getting all fields from metadata, checking their type if they date/datetime, converting datetime object to string just for uniqueness validation seriously break for SOLID?

Replace datetime with array and you have the same problem I described using a recommended example from the manual.

@Jurigag
Copy link
Contributor

Jurigag commented Jul 22, 2016

We could just add convert parameter which could be just anonymous function which would be bind to uniquness validator, which would accept parameters you are passing and you can do anything with them and return them and they are will be used. Like for example:

 $validator->add(
            ["name", "deleted"],
            new Uniqueness([
                "message" => "The name must be unique.",
                "convert" => function($value) {
                        $value['deleted'] = $value['deleted']->format("Y-m-d H:i:s");
                        return $value;
                }
            ])
        );

@basilfx
Copy link
Contributor Author

basilfx commented Jul 22, 2016

Awesome, that would definitely fix the problem while keeping everything at one place.

I don't mind to implement this. Can do this on monday.

@Jurigag
Copy link
Contributor

Jurigag commented Jul 22, 2016

Yea but it's kind of more changes then you think since validators are using some method from Phalcon\Validation\Validator to getValue of ONE field. And uniqueness validator is just doing foreach through fields - and we want value of all those fields in our anonymous function as one parameter - array if we passed array as fields or just single field if we passed a string.

We should just get all values before starting the loop, and then in loop work on those values.

Like before this loop - https://github.com/phalcon/cphalcon/blob/2.1.x/phalcon/validation/validator/uniqueness.zep#L109 create some array with values, bind our function to object, call it and set new values and work on those values in this loop from this line(so instead of validation->getValue(field) just values[field].

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants