Type Validator not working because of a cast in the mutator #283

Closed
ghost opened this Issue Feb 6, 2012 · 4 comments

Projects

None yet

3 participants

@ghost

Hi everybody,
i've found that if you have a column defined as type correspondig to a php primitive in the schema.xml (FLOAT, INT, DOUBLE, BOOLEAN) and you add a type validator, it doesn't work because of a cast added in the addDefaultMutator method of the PHP5ObjectBuilder.php

Example

<column name="pf1_eurkWh_sim" phpName="Pf1EurkwhSim" type="FLOAT" sqlType="float unsigned" required="true"/>
<validator column="pf1_eurkWh_sim">
        <rule name="type" value="float" message="Il prezzo deve essere un valore decimale separato dal ' . ' " />
</validator>

This will generate the setter down below

public function setPf1EurkwhSim($v)
    {

                if ($v !== null) {
            $v = (double) $v;
        }

        if ($this->pf1_eurkwh_sim !== $v) {
            $this->pf1_eurkwh_sim = $v;
            $this->modifiedColumns[] = FarmSimulazioneoffertaEnergiaPeer::PF1_EURKWH_SIM;
        }

        return $this;
    } // setPf1EurkwhSim()
}

So if you set the value using $obj->setPf1EurkwhSim("Hello") and then you call the validate() method it will return true because of the cast of the string "Hello" to a double (so = 0).

Since i wanted to catch the error in this case i have updated the addDefaultMutator() method

//File: propel/generator/lib/builder/om/PHP5ObjectBuilder.php (line: 1739)

protected function addDefaultMutator(&$script, Column $col)
    {
        $clo = strtolower($col->getName());

        $this->addMutatorOpen($script, $col);

        // Perform type-casting to ensure that we can use type-sensitive
        // checking in mutators.
                // @modified added "&& is_numeric(\$v)" into the if 
                // to fix the TYPE VALIDATOR rule in the schema.xml file
        if ($col->isPhpPrimitiveType()) {
            $script .= "
        if (\$v !== null && is_numeric(\$v)) {
            \$v = (".$col->getPhpType().") \$v;
        }
";
        }        

        $script .= "
        if (\$this->$clo !== \$v) {
            \$this->$clo = \$v;
            \$this->modifiedColumns[] = ".$this->getColumnConstant($col).";
        }
";
        $this->addMutatorClose($script, $col);
    }

I hope it could be useful. If there are mistakes or known issues with this, please let me know.
Thank you.

@willdurand
Propel member

@leandroluccerini thanks, could you write a PR with you fix?

@havvg havvg added a commit to Ormigo/Propel that referenced this issue Jan 2, 2013
@havvg havvg Merge branch 'master' into ormigo
* master:
  Fixes #345
  Allow the QuickGenerator to use another Platform
  Change fallback order in getTableByPhpName(). Fixes #385
  Fix initialization of internal iterator for getRelCol. fix #460
  Add a test for initialization of internal iterator for related object collection getter fix #460
  Fix PR #544
  teaching propel to clear UP and DOWN when calling clearAllReferences(true) we have infinite recursion prevention already covered also we add an optional param to clearInstancePool. if passes as true it will clearAllReferences(true) on every instance before clearing it.
  Fix CS
  Fix cast in setters. Should fix #283
  Fix inconsistency for BIGINT. Fixes #459
  Use model prefix in QueryInheritanceBuilder, fixes #542
  fix variable name on boolean filter methods
  Tests for improved ENUM handling
  Improved getValueSet() method, added ENUM getters for SQL value, set Query filter to use SQL getters
  Add PHP 5.5 to travis config
  Update generator/lib/builder/om/PHP5PeerBuilder.php
  sqlType="enum(..)" now set the valueSet attribute it is not required to use Propel::ENUM as type
7d94f67
@dancy

This change adds another bug. If you pass an object to the setter of a string field it would not be casted as string and thus the behavior of the field might become unpredictable. This might happen if a column getter was overriden either by user or by the behavior. It brings me much trouble.

@staabm
Propel member

@dancy could you provide a unit test or a more concrete example?

@dancy

I don't know how to create a unit test for Propel correctly. If anyone could explain me it would be great.
Imagine you have a model "Image" that has field called "filename" that stores the name of the uploaded file. You want to use the value of the field in two ways. First, you want to check that the file exists on the server's filesystem. But on the other hand you want to provide user a URI for downloading the file. This can be solved by overriding a "getFilename" function. The new function will return an instance of class, providing described functions. Let's call it "ModelResource". So, you can call $image->getFilename()->path() to retrieve full path to file on filesystem and $image->getFilename()->url() to retrieve an url to the file. The problem rises when you attempt to submit a Symfony 1.4 form for the "Image" model with "filename" parameter not set. Consider you have a Symfony form "ImageForm" with field "image". During the form initialization the form calls the "updateDefaultsFromObject" function. It iterates through object fields and calls a getter function for each of them. After calling this function the default value of "image" field will be an instance of "ModelResource" class as the getter was overriden. Then if no value for "image" field was provided in the request it will remain untouched during the form processing. When you attempt to save such a form, an "updateObject" function is called which by itself calls the "fromArray" function of "Image" model. "fromArray" function itself calls the "setFilename" function. But as the default value remains untouched, an instance of ModelResource will be passed to "setFilename" function, which is neither a string, nor a numeric value, It is a class instance. So the "collFilename" variable will now store an instance of 'ModelResource'. For the model to be successfully saved this value must be converted to string, that's why the 'ModelResouce' class implements a "__toString" function. It's default behavior is to return the value of the 'filename' field of 'Image' model. But after this patch only numeric values are casted to string upon setting field value. That's why when it tries to read the value of the field it gets another instance of 'ModelResource' object, tries to cast it to string and falls into infinite recursion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment