Skip to content

Commit

Permalink
dev: Moved getMaxId to LSActiveRecord, added option for different fie…
Browse files Browse the repository at this point in the history
…ld / force refresh
  • Loading branch information
mennodekker committed Jul 12, 2013
1 parent cdd660f commit e32991b
Show file tree
Hide file tree
Showing 4 changed files with 38 additions and 42 deletions.
2 changes: 1 addition & 1 deletion application/helpers/admin/activate_helper.php
Expand Up @@ -26,7 +26,7 @@ function fixNumbering($fixnumbering, $iSurveyID)
LimeExpressionManager::RevertUpgradeConditionsToRelevance($iSurveyID);
//Fix a question id - requires renumbering a question
$oldqid = (int) $fixnumbering;
$lastqid=Questions::model()->getMaxId();
$lastqid=Questions::model()->getMaxId('qid', true); // Always refresh as we insert new qid's
$newqid=$lastqid+1;

// Not sure we can do this in MSSQL ?
Expand Down
38 changes: 37 additions & 1 deletion application/models/LSActiveRecord.php
Expand Up @@ -103,6 +103,42 @@ public function beforeDelete()
{
$result = App()->getPluginManager()->dispatchEvent(new PluginEvent('before'.get_class($this).'Delete', $this));
return parent::beforeDelete();
}
}

/**
* Return the max value for a field
*
* This is a convenience method, that uses the primary key of the model to
* retrieve the highest value.
*
* @param string $field The field that contains the Id, when null primary key is used if it is a single field
* @param boolean $forceRefresh Don't use value from static cache but always requery the database
* @return false|int
*/
public function getMaxId($field = null, $forceRefresh = false) {
static $maxIds = array();

This comment has been minimized.

Copy link
@olleharstedt

olleharstedt Mar 17, 2023

Collaborator

This one causes problem in our unit tests, since we run multiple API calls in one "request".

My question: Do we need $maxIds to cache the result here? It's never called in a loop, what I can see, and it wasn't cached before this commit. Speed gain in minimal, but unit test confusion is maximum. ^^

@Shnoulle Any opinion about this piece of code?

This comment has been minimized.

Copy link
@Shnoulle

Shnoulle Mar 17, 2023

Collaborator

multiple API calls

remote control ?

Why not unlog/log in between each action done by API ? In general it's what happen.

BUT : this static maxIds must depends on $field too … if it's not the same $field : it's another $maxIds AND it must be linked to the table too.

Seems API control found an issue .

In general : i think we need :

  1. more static (see the issue about speed i report for 5.X (and another report same for 5.X too))
  2. more cache (really more)

This comment has been minimized.

Copy link
@olleharstedt

olleharstedt Mar 17, 2023

Collaborator

If we add more cache, it must be easy to clear cache too. Having a single static variable in a method like this is not easy to clear. 😛

This comment has been minimized.

Copy link
@Shnoulle

Shnoulle Mar 18, 2023

Collaborator

Oh yes :)

But here : there are an issue : static didn't use $field

It's because we use another table in same request ? Then : need to move to a Class static variable.


if (is_null($field)) {
$primaryKey = $this->primaryKey();
if (is_string($primaryKey)) {
$field = $primaryKey;
} else {
// Composite key, throw a warning to the programmer
throw new Exception(sprintf('Table %s has a composite primary key, please explicitly state what field you need the max value for.', $this->tableName()));
}
}

if ($forceRefresh || !array_key_exists($field, $maxIds)) {
$maxId = $this->dbConnection->createCommand()
->select('MAX(' . $this->dbConnection->quoteColumnName($field) . ')')
->from($this->tableName())
->queryScalar();

// Save so we can reuse in the same request
$maxIds[$field] = $maxId;
}

return $maxIds[$field];
}

}
18 changes: 0 additions & 18 deletions application/models/Question.php
Expand Up @@ -96,24 +96,6 @@ public function rules()
);
}

/**
* Return the max id (primary key)
*
* Actually used in activate_helper fixNumbering function
* Don't use static because can be call after adding a question
*
* @since 130711
* @return false|int
*/
public function getMaxId()
{
$maxId = $this->dbConnection->createCommand()
->select('MAX(qid)')
->from($this->tableName())
->queryScalar();
return $maxId;
}

/**
* Rewrites sort order for questions in a group
*
Expand Down
22 changes: 0 additions & 22 deletions application/models/SurveyDynamic.php
Expand Up @@ -261,28 +261,6 @@ public function exist($srid)
return $exist;
}

/**
* Return the max id (primary key)
*
* This is used in export, when using the record id instead of the row number (count of records)
*
* @staticvar null $maxId
* @return false|int
*/
public function getMaxId()
{
static $maxId = null;

if (is_null($maxId)) {
$maxId = $this->dbConnection->createCommand()
->select('MAX(' . $this->primaryKey() . ')')
->from($this->tableName())
->queryScalar();
}

return $maxId;
}

/**
* Return next id if next response exist in database
*
Expand Down

0 comments on commit e32991b

Please sign in to comment.