Doctrine 1
  1. Doctrine 1
  2. DC-538

Doctrine_Table::enumIndex() and Doctrine_Table::enumValue() should return easily distinguishable value (e.g. false) when index/value is not found

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.2.1
    • Fix Version/s: 1.2.3
    • Component/s: None
    • Labels:
      None
    • Environment:
      Any

      Description

      At the moment:

      • Doctrine_Table::enumIndex($fieldName, $value) returns $value specified as a parameter if no index for it is found
      • Doctrine_Table::enumValue($fieldName, $index) returns $index specified as a parameter if no value for it is found

      This actually makes writing code which deals with enums a bit difficult as one would like to detect incorrect parameter as soon as possible (e.g. not when Doctrine validation kicks in and checks that enum value is incorrect).

      Current algorithm that returns index or value is not sufficient to determine error in situation when index=value, e.g. enum values are [0, 1, 2, 3]. Calling e.g. enumValue('sample', 4) would yield 4 - so it makes difficult to determine whether it's correct value.

      I hope this makes sense, let me know if anything is unclear.

      The workaround for this is to check whether mapped index/value exists in array returned by Doctrine_Table::getEnumValues(), but this is an extra unnecessary check.

      So instead of writing:

      $index = 5;
      $someTable = Doctrine_Core::getTable('Sample');
      $value = $someTable->enumValue('sample', $index);
      
      $values = $someTable->getEnumValues('sample');
      if (in_array($value, $values) == false)
      {
        throw new UnexpectedValueException('illegal index specified!');
      }
      

      I'd like to write:

      $index = 5;
      $value = Doctrine_Core::getTable('Sample')->enumValue('sample', $index);
      if ($value === false)
      {
        throw new UnexpectedValueException('illegal index specified!');
      }
      

      Simpler, eh?

      1. Column.patch
        0.3 kB
        Michal Olszewski
      2. DC-538_fix.patch
        4 kB
        Michal Olszewski
      3. Table.patch
        0.2 kB
        Michal Olszewski

        Activity

        Hide
        Michal Olszewski added a comment -

        Please find patches for Doctrine/Table.php and Doctrine/Column.php against their versions from rev. 7298

        Patches includes proposed change (returning false instead of original parameter).

        Interestingly, Doctrine_Column::enumIndex() returns FALSE when index is not found for specified value as it uses array_search().
        So - another reason why this processing should be unified

        Show
        Michal Olszewski added a comment - Please find patches for Doctrine/Table.php and Doctrine/Column.php against their versions from rev. 7298 Patches includes proposed change (returning false instead of original parameter). Interestingly, Doctrine_Column::enumIndex() returns FALSE when index is not found for specified value as it uses array_search(). So - another reason why this processing should be unified
        Hide
        Jonathan H. Wage added a comment -

        Hi, did you run your patch against the test suite? It breaks several tests so we cannot apply it. Please re-open if you have more information and a new patch. Thanks, Jon

        Show
        Jonathan H. Wage added a comment - Hi, did you run your patch against the test suite? It breaks several tests so we cannot apply it. Please re-open if you have more information and a new patch. Thanks, Jon
        Hide
        Michal Olszewski added a comment -

        Hi,

        I think I've nailed this one down.

        First of all, Doctrine_Table::enumValue() is used incorrectly in two places:

        1. Doctrine_Record::unserialize() has 'special case' for enums, which performs following conversion - from enum's 'index' to 'value':
          $this->_table->enumValue($k, $this->_data[$k]);

          Now this doesn't make sense because Doctrine_Record::serialize() does not store enum's 'index' but 'value' so Doctrine_Table::enumValue() always receives 'value'. Previous implementation simply returned the same 'value' in case when check:

          isset($this->_columns[$columnName]['values'][$index])

          failed. And it was always failing because 'index' is actually one of the 'values'.

        2. Doctrine_Table::prepareValue() is the same case - it receives 'value' and not 'index' so there is no point in calling Doctrine_Table::enumValue().

        Please note I assumed two things:

        1. Correct use of 'emulated' enums (second case) is to set them as strings, not integer values (it seemed reasonable after looking at test cases and documentation). Also: 'emulated' enums are stored in DB as text, not integers.
        2. 'Native' enums are not mapped so they don't change anything.

        Under these assumptions I propose two changes:

        1. Add special case for enums in Doctrine_Record::serialize() so enum value is mapped to enum index
        2. Remove special case for enums in Doctrine_Table::prepareValue() and always return $value parameter as it was specified (move it to 'string'/'int' case)

        I've prepared patch for these changes and will attach it soon.

        Thanks.

        Show
        Michal Olszewski added a comment - Hi, I think I've nailed this one down. First of all, Doctrine_Table::enumValue() is used incorrectly in two places: Doctrine_Record::unserialize() has 'special case' for enums, which performs following conversion - from enum's 'index' to 'value': $this->_table->enumValue($k, $this->_data[$k]); Now this doesn't make sense because Doctrine_Record::serialize() does not store enum's 'index' but 'value' so Doctrine_Table::enumValue() always receives 'value'. Previous implementation simply returned the same 'value' in case when check: isset($this->_columns[$columnName]['values'][$index]) failed. And it was always failing because 'index' is actually one of the 'values'. Doctrine_Table::prepareValue() is the same case - it receives 'value' and not 'index' so there is no point in calling Doctrine_Table::enumValue() . Please note I assumed two things: Correct use of 'emulated' enums (second case) is to set them as strings, not integer values (it seemed reasonable after looking at test cases and documentation). Also: 'emulated' enums are stored in DB as text, not integers. 'Native' enums are not mapped so they don't change anything. Under these assumptions I propose two changes: Add special case for enums in Doctrine_Record::serialize() so enum value is mapped to enum index Remove special case for enums in Doctrine_Table::prepareValue() and always return $value parameter as it was specified (move it to 'string'/'int' case) I've prepared patch for these changes and will attach it soon. Thanks.
        Hide
        Michal Olszewski added a comment - - edited

        Attaching patch (DC-538_fix.patch), including:

        • Doctrine_Table::enumValue() and Doctrine_Table::enumIndex() now returns false if parameter cannot be mapped
        • Doctrine_Table::prepareValue() does call Doctrine_Table::enumValue() for enum type
        • Doctrine_Record::serialize() serializes enum's index, not value, so unserialization works fine

        Please apply it if you agree with my previous comment.

        Show
        Michal Olszewski added a comment - - edited Attaching patch ( DC-538 _fix.patch), including: Doctrine_Table::enumValue() and Doctrine_Table::enumIndex() now returns false if parameter cannot be mapped Doctrine_Table::prepareValue() does call Doctrine_Table::enumValue() for enum type Doctrine_Record::serialize() serializes enum's index, not value, so unserialization works fine Please apply it if you agree with my previous comment.

          People

          • Assignee:
            Jonathan H. Wage
            Reporter:
            Michal Olszewski
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: