enum mutator generator - modification detection quirk #264

Open
pkschweiger opened this Issue Jan 19, 2012 · 4 comments

Comments

Projects
None yet
3 participants

enum columns php type is string and PHP5ObjectBuilder's hydration generator casts member variable to string.

enum mutator function generator translates string values via array_search() on the type's value set. array_search() on the numerically indexed value set returns an integer, which in turn prevents the check for gratuitous call to the mutator from getting it's job done.

quick fix in PHP5ObjectBuilder::addEnumMutator() is a cast of array_search() result to column's php type (string) as the hydrator does.

also the in_array() call for validation, followed by the array_search() to translate the value seems rather inefficient, here is a proposed addEnumMutator() method that addresses both issues:

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

        $script .= "
        if (\$v !== null) {
            \$valueSet = " . $this->getPeerClassname() . "::getValueSet(" . $this->getColumnConstant($col) . ");

            \$idx = array_search(\$v, \$valueSet);
            if (\$idx === false) {
                throw new PropelException(sprintf('Value \"%s\" is not accepted in this enumerated column', \$v));
            }
            \$v = (".$col->getPhpType().")\$idx;
        }

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

willdurand commented Jan 26, 2012

Hi, can you write a patch with some unit tests ?

Member

fzaninotto commented Feb 13, 2012

I like the fix but I don't know what's the bug? Can you provide an example case where not casting to string creates a problem?

Hi Guys.

Apologies about my lame response time.

I browsed around the existing unit tests a bit to take a stab at packaging this up in the way willdurand requested. I found nothing less than an existing unit test, referencing #139 - opened 3 months before I reported this issue. The fix to 139 actually changed the PropelTypes::ENUM_NATIVE_TYPE from 'string' to 'int' and passes GeneratedObjectEnumColumnTypeTest::testSetterWithSameValueDoesNotUpdateObject().

The 'quick fix' cast via getPhpType() doesn't even result in a string cast anymore. It winds up being a no-op, because the native type is already int.

I feel fzaninotto's fix by changing the internal type to in is cleanest and most effecient since value sets are presented as arrays numerically indexed by the interpreter.

I have used hacks found on the mailing lists to coax propel into using mysql native enum types by specifying a sqlType of ENUM('foo,'bar,'baz,) in the schema definition, then rigging the valueSets to manually key the array with the string constants themselves. At the time I even put a todo list item into my code:

class SomePeer extends BaseSomePeer
{
    /**
     * @todo this should really become a propel orm patch
     *  - to affect generation in the base peer class instead of an
     *    override in stub
     *  - remove requirement for redundant sqlType declaration in schema file
     */
    protected static $enumValueSets = array(
        self::COL => array(
            self::COL_FOO => self::COL_FOO,
            self::COL_BAR => self::COL_BAR,
            self::COL_BAZ => self::COL_BAZ
        );
    );
}

This hack actually depends on the internal type being a string, and will break down when I next upgrade propel. I don't typically use native enum type (can be a real pain to alter on large tables), but have selected the type for clarity where database tables are fed by external systems far removed from php even.

At the time the native enum type hack was proposed on the mailing list, propel expressed interest in a native enum feature to the hack author. I'm confident I can knock out schema definition and generation components, but I have no clue what (if any) repercussions native sql enum has on migrations. Migrations could render me in over my head in terms of time I can devote to it.

If you are still interested, and think it's worth the effort for the 1.x branch, I will take a stab at it - it's on my ever growing todo list after all.

Owner

willdurand commented Aug 7, 2012

Do you want to write a PR?

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