Model_Temporal: Multiple functions don't work when primary key is changed from 'id'

[willpoorman]

If a model exists where 'id' is not a column and is replaced with any number of other columns as the primary key (disregarding the necessary temporal_start and temporal_end columns that are part of the pk), the following functions fail, since their queries hardcode the use of the 'id' column:

find_revisions_between, find_revision, restore, and purge

Say that Model_Foo extends Model_Temporal and it as a primary key of:

protected static $_primary_key = array('foo', 'bar', 'temporal_start', 'temporal_end');

So instead of "id", it uses "foo" + "bar". Well the find() function handles this just fine when you pass an array for a compound key find(array('foo' => 1, 'bar' => 2)):

switch ($id)
{
    ...
    default:
        $id = (array) $id;
        $count = 0;
        foreach(static::getNonTimestampPks() as $key)
        {
            $options['where'][] = array($key, $id[$count]);

            $count++;
        }
        break;
}

But the column "id" is hard coded into the query in the functions listed at the top: Example issue from find_revision():

$query = static::query()
    ->where('id', $id)
    ->where($timestamp_start_name, '<=', $timestamp)
    ->where($timestamp_end_name, '>', $timestamp);

Example issue from restore():

$activeRow = static::find('first', array(
        'where' => array(
            array('id', $this->id),
            array($timestamp_end_name, $max_timestamp),
        ),
    ));

Suggested fix: Do something similar to what find() does to deal with compound keys. Here's a fix I have written up for find_revision:

public static function find_revisions_between($id, $earliestTime = null, $latestTime = null)
{
    ...
    static::disable_primary_key_check();

    //Select all revisions within the given range.
    $query = static::query()
        ->where($timestamp_start_name, '>=', $earliestTime)
        ->where($timestamp_start_name, '<=', $latestTime);

    $primary_keys = static::getNonTimestampPks();

    foreach($primary_keys as $key)
    {
        if(count($primary_keys) == 1)
        {
            $query->where($key, $id);
        }
        else
        {
            $query->where($key, $id[$key]);
        }
    }

    static::enable_primary_key_check();
    ...
}

This fix should be able to be thrown into find_revision and find_revisions_between. It will probably need to be bit different for restore and purge but it should be similar. I'll try to implement the fix and create a pull request from my fork when I get the chance. But wanted to make sure the issue was known before I forget about it.

[WanWizard]

I personally ever used the model, but I was aware of some hardcoding.

I did a quick search, but this seems to be the only place:

./classes/model/temporal.php:   protected static $_primary_key = array('id', 'temporal_start', 'temporal_end');
./classes/model/temporal.php:           ->where('id', $id)
./classes/model/temporal.php:           ->where('id', $id)
./classes/model/temporal.php:                   array('id', $this->id),
./classes/model/temporal.php:       return $query->where('id', $this->id)

looking forward to a PR. :+1: