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. 👍