Fixed issue #19555: afterFindSurvey gets triggered excessively

[TonisOrmisson]

There is some additional discussion regarding this fix in #3816

The afterFindSurvey event gets triggered each time survey is found found via Survey::findByPk(), but as well always when survey is found via relations eg $question->survey.

With a survey page of multiple questions and even sub-questions the event afterFindSurvey might get triggered tens of times on one survey page load.

There is a discussion whether this is a bug or a feature.

1. It's a feature view
   afterFindSurvey event is tightly related to instantianting any survey object, also meaning it must be triggered after initializing a survey via relations or any time a survey object is populated. In this case it is not a bug and we should not fix it, this PR should be closed.

2. It's a bug view
   The event manual states that the event is for:

to be able to modify/override some display parameters for current display session

This means the whole display session (survey page) - meaning it does not need to be triggered tens of times during a a page load.
It is not related to initializing any Survey object it is meant specifically to trigger after Survey::findByPk(). In this case It's a bug and this PR will move the triggering essentially from populating any survey record to specifically only if Survey::findByPk() returns a new object (not stored also via Survey::$findByPkCache)

[Shnoulle]

The function afterSurveyFind do some other things

For example

LimeSurvey/application/models/Survey.php

Line 588 in c2b4fd9

$this->setOptions($this->gsid);

I think it's better too to have it only one time but

• When we get it via $question->survey : options are set
  • currently
  • after this commit

Need some unit test too to check if

1. plugin event can update something
2. template is fixed
3. options are set

[gabrieljenik]

The afterFind LS plugin event is tightly related to the onAfterFind Yii event.
https://www.yiiframework.com/doc/api/2.0/yii-db-activerecord#afterFind()-detail

What is being suggested here looks more to me like:

• Using the same afteFind event name.
• For a closely related, but different, purpose. If I understand correctly the afterFind Yii event will be triggered for many find situations. The new will only be triggered on the afterFindByPk.
• This may break compatibility backwards facing.

I would just:

• Create a new PluginEvent afterInit.
• Eventually, with time, review if the afterFind plugin event should be deprecated

Also, of course, clarify the situation with the documentation.

[TonisOrmisson]

#3816 (comment)
I will comment the documentation comment here

It seems to me the issue should be solved starting from the stated purpose of the event. Why this event exists, what is the purpose of the event? The description of the event in the documentation could be old version/date-wise but there might not have anything significant changed code-wise for that specific event after documenting it.

I'm not sure which way is better to go, it depends on how LS states the purpose of the event. Currently I see the fact is that the stated purpose and actual behavior do not match. People using the event for the stated functionality end up calling the event excessively possibly without realizing it (I know I was).

'afterInit' - you mean afterInit Survey model? This seems very similar to the current afterFindSurvey essentially. I personally do not see use for that and have no comment on possible afterInitSurvey.

[gabrieljenik]

People using the event for the stated functionality end up calling the event excessively possibly without realizing it (I know I was).

Agree. So I would create a new event so people can start moving to use that event that better fits their situation.

'afterInit' - you mean afterInit Survey model? This seems very similar to the current afterFindSurvey essentially. I personally do not see use for that and have no comment on possible afterInitSurvey.

Similar, but would only be triggered once.
Not sure about the name. Probably the name will depend on when is technically being triggered.

[TonisOrmisson]

Similar, but would only be triggered once.

How once? Init gets triggered even more. I attached a log to Survey init() and afterFindSurvey and loaded one survey page. The result looks like that :

That for sure will not solve the excessive triggering issue

[Shnoulle]

The issue is this one : With 3.X and before : LimeSurvey don't use $question->survey (and no pjax/ajax call (not a lot))

Then when using survey->findByPk : we call it one time when we work on a survey.

Seems the new versions : 5 and master have

• Call with $question->survey
• Called by ajax for one page

My opinion :

1. afterSurveyFind event can update any attribute of Survey object
2. afterSurveyFind event musts be called one time for a page call : return same Survey object (static)
3. the survey object call by question->survey must return the same Survey object

Maybe we need to update the way Yii return $question->survey ? If we have a $question->getSurvey : it happen when we use $question->survey ?

There the ajax call : multiple call for one HTML page, but i think we really reduce here.

[gabrieljenik]

How once? Init gets triggered even more.

I mean the new event should be triggered much less than the current afterFind. Expectedly, once.

Not sure about the name. Probably the name will depend on when is technically being triggered.

I if ti shall be used for survey taking process, we could name it beforeSurveyLoadSurvey
Then, yes, I expect the new event ot be called once

[gabrieljenik]

afterSurveyFind event can update any attribute of Survey object

I think that's already done.

afterSurveyFind event musts be called one time for a page call : return same Survey object (static)

I wouldn't update how this works. I would create a new event for it.
Then developers should move their plugins as they seem appropriate

[Shnoulle]

I wouldn't update how this works. I would create a new event for it.
Then developers should move their plugins as they seem appropriate

It work like that when it was created and updated : you have to reset the static variable in beforeSureyPage to allow update (i have a plugin with this).

The 5.X system work not like 3.X system then.