I would go with:

1- Add a new optional parameter to getMaxId
public function getMaxId($field = null, $forceRefresh = false, $clear=false)

2 - Create LSActiveRecord::refreshMetadata() (overload of CActiveRecord).
Make the refreshMatedata to call getMaxId using the clear parameter.

DC said...

a lot must be automatic. If not : it's unusuable

For example, you can do

$maxId1 = \Group::model()->getMaxId();
$maxId2 = \QuestionGroup::model()->getMaxId();
$maxId3 = \Token::model(123)->getMaxId();
$maxId4 = \SurveyDynamic::model(123)->getMaxId();
$maxId5 = \SurveyDynamic::model(321)->getMaxId();
$maxId6 = \Response::model(321)->getMaxId();

You must have the real $maxId at each time.

And more

$maxId2 = \QuestionGroup::model()->getMaxId();
$oQuestionGroup = new \QuestionGroup());
…
$oQuestionGroup->save();
$maxId3 = \QuestionGroup::model()->getMaxId();

DC Said

A question : did we really need this one ? Maybe just delete this statci var

I think it's best to optimize other request (and use static or cache)

https://bugs.limesurvey.org/view.php?id=18473
https://bugs.limesurvey.org/view.php?id=18472
etc …

There are an issue in this code … there are an easy way to fix : why you don't want to fix it ?

Not so easy … ;)

haha I was trying to port the conversation :)

Draft broken test : https://github.com/LimeSurvey/LimeSurvey/pull/3006

I like to keep some statci var when multiple load too

For example : wen you ask Options for Survey #1 in Group #1 : you ask option in Survey #1 + option on Group #1 + Global option , the n if you as after option in Survey #2 : i like to keep option on Group #1 + Global option

Same for Template option (where you can have more than on Global option …)

maxIds is a good test for all of this ;)

Is it ok if I take this?
I will do a test script as well as the fix

I think i get it
https://github.com/LimeSurvey/LimeSurvey/pull/3006

I need to add same for minIds

OK ?

5.X : https://github.com/LimeSurvey/LimeSurvey/pull/3006 (already reviewed)
master : https://github.com/LimeSurvey/LimeSurvey/pull/3209

Di i create test ? I create only if confirmed , don't want to create and must fix again in 6 month.

@ollehar : did i create test ? If auto test is OK : can merge ?

Are you sure this is not premature optimization? https://softwareengineering.stackexchange.com/questions/80084/is-premature-optimization-really-the-root-of-all-evil

Alternative solution : reset if $dynamicId are updated.

What is your proposed solution ?

Issue still there.

The solution with less code but worse performance is to never cache it in a local variable.

But the trade-off with simpler code is worth it if performance is only very slightly affected. Depends on how often the code is called, if it's in a tight loop or so.

Solutions

1. Remove static var
2. Get static var but reset if dynamic id was update
3. Current PR

I can do 2 here.

Maybe 1 is OK here

controllers/admin/Export.php:            $iMaximum = SurveyDynamic::model($iSurveyID)->getMaxId();
helpers/remotecontrol/remotecontrol_handle.php:        if (!($maxId = SurveyDynamic::model($iSurveyID)->getMaxId(null, true))) {
helpers/remotecontrol/remotecontrol_handle.php:        if (!($maxId = SurveyDynamic::model($iSurveyID)->getMaxId())) {
helpers/admin/activate_helper.php:    $iMaxQID = Question::model()->getMaxId('qid', true); // Always refresh as we insert new qid's
models/LSActiveRecord.php:    public function getMaxId($field = null, $forceRefresh = false)

Yes, assuming the export code is not in a loop?

No,

and remove the $forceRefresh then …

Yep, not needed. :)

Sorry got lost. What's wrong with Denis solution?

My initial idea (https://bugs.limesurvey.org/view.php?id=18699#c74246) was to:
1- Add a new optional parameter to getMaxId
public function getMaxId($field = null, $forceRefresh = false, $clear=false)

2 - Create LSActiveRecord::refreshMetadata() (overload of CActiveRecord).
Make the refreshMatedata to call getMaxId using the clear parameter.
refreshMetadata is already called inside the dynamic model methods when changing dynamicIds.

That can be complemented by making this variable inside the method static $maxIds = []; to be declared as a class attribute.
That would make things more flexible in the future and create methods which are only for refreshing or clearing, ....

By looking at Denis solution, now in DEV branch, I would also add some more stuff like:

• adding getDynamicId() to some classes
• making more classes to derive from Dynamic

@gabrieljenik : removing static var and no issue.

Since don't seems to be used more than one time in action : we don't need a static var

Could be. It would be inline with simplifying. It would remove the cache.

Still, what's wrong with Denis solution?

Is this ready for testing or not? I can't tell, because the discussion is still ongoing and this is tagged with "in code review", but PRs are tagged with "Code review done" :/
If this is ready to test, please provide me with some good instructions, plus the script that is mentioned in "how to reproduce".

@DenisChenu Is this ready for testing or not? I can't tell, because the discussion is still ongoing and this is tagged with "in code review", but PRs are tagged with "Code review done" :/
If this is ready to test, please provide me with some good instructions, plus the script that is mentioned in "how to reproduce".