New feature: added beforeGetTemplate plugin event

[TonisOrmisson]

I am proposing to add a plugin event to be able to change the survey template.

I have done this since via the event afterFindSurvey, but since there is way too much Survey::findByPk($sid) calls, using afterFindSurvey event for virtually any type of plugin manipulation seems to generate a huge overhead.

Discussion is very welcome :)

[TonisOrmisson]

Just a note, when logging the amount of afterFindSurvey events on survey page loads, depending on the page there is 5-40+ afterFindSurvey events fired on one page load.

If one adds a plugin doing anything on that event, the amount of possibly meaningless processing can be huge.

[TonisOrmisson]

Seems like ~5-6 afterFindSurvey calls per questions rendered. Having 10 questions on a page calls afterFindSurvey 60+ times on one page render.

[TonisOrmisson]

Ill try to find some time to go a bit over places like that in a separate pr:

[Shnoulle]

Seems like ~5-6 afterFindSurvey calls per questions rendered. Having 10 questions on a page calls afterFindSurvey 60+ times on one page render.

What ? This must be fixed : Survey::findByPk must call

LimeSurvey/application/models/Survey.php

Line 1005 in 9b6a908

self::$findByPkCache[$pk] = $model;

I think the issue is here before ? No ?

Maybe report it separately ?

[Shnoulle]

getTemplateConfiguration already must be improved …

Working at c6bed76 i see we need more optimization on TemplateCOnfiguration system.

Here i think it's more in

LimeSurvey/application/models/Template.php

Line 550 in 9b6a908

public static function getInstance($sTemplateName = null, $iSurveyId = null, $iSurveyGroupId = null, $bForceXML = null, $abstractInstance = false, $last = false)

you need an event ?

[Shnoulle]

Need a mantis issue please

[TonisOrmisson]

Thanks for looking into this.

I tested that Survey $findByPkCache. It seems to work partially, turning the cached return off results in loads of more event calls. I tested this quite a bit, and it seems onAfterFind event gets also triggered by yii internally after fetching via relations. So whenever you call $question->survey the foundByPk() gets NOT called, but internally Surveys onAfterFind GETS called (also triggering the LS events).

Nevertheless, it seems unreasonable that switching a template use case is only workable via afterFind (onAfterFind) event gets called tens of times. To be honest, looking at this I would discourage to hook any plugin events to onAfterFind since they seem to be happening a LOT, maybe even mark this as not suggested plugin event.

@Shnoulle you mean mantis issue regarding what here, the afterFind being triggered too much or what?

My aim here is to get a plugin event that more reasonably allows rewriting the template in case of displaying the questionnaire.

Using TemplateConfiguration::getInstance() will not work for that $bForceXML is enabled?

[Shnoulle]

Thanks for looking into this.

I tested that Survey $findByPkCache. It seems to work partially, turning the cached return off results in loads of more event calls. I tested this quite a bit, and it seems onAfterFind event gets also triggered by yii internally after fetching via relations. So whenever you call $question->survey the foundByPk() gets NOT called, but internally Surveys onAfterFind GETS called (also triggering the LS events).

Good catch ! The original issue are maybe here !

Nevertheless, it seems unreasonable that switching a template use case is only workable via afterFind (onAfterFind) event gets called tens of times. To be honest, looking at this I would discourage to hook any plugin events to onAfterFind since they seem to be happening a LOT, maybe even mark this as not suggested plugin event.

@Shnoulle you mean mantis issue regarding what here, the afterFind being triggered too much or what?

Multiple event happen here i think. Maybe we need to move the event elsewhere ?

A feature for the template is needed to (in my opinon).

My aim here is to get a plugin event that more reasonably allows rewriting the template in case of displaying the questionnaire.

Using TemplateConfiguration::getInstance() will not work for that $bForceXML is enabled?

Don't know … untested here. But with bForceXML : you don't have any configuration, only the XML configuration. It's more a dev/debug system if i don't make error (or when setup a new theme).

[TonisOrmisson]

OK. I understand that there is agreement in the idea that a plugin hook is needed/suitable for the intended purpose and it is a question of where is the most appropriate place to have that.

@Shnoulle do I get your opinion right that you would prefer to have it in Template::getInstance() instead of the initially offered Template::getTemplateConfiguration() ??

I must say I don not have a strong feeling which suits better, I can refactor that for the getInstace() instead if that looks more suitable.

[Shnoulle]

OK. I understand that there is agreement in the idea that a plugin hook is needed/suitable for the intended purpose and it is a question of where is the most appropriate place to have that.

Yes : its' still a good idea

I must say I don not have a strong feeling which suits better,

Same for me. ping @gabrieljenik for second advice ?

What we must avoid it's event happen multiple time for the same (public) page: event must happen one time only and data stay like after the event.

About bForceXML : it's already broke SurveyTheme settings : settings came from XML not from DB, for example for logo. I really think we must not really care about bForceXML

[gabrieljenik]

I am proposing to add a plugin event to be able to change the survey template.

Why not https://manual.limesurvey.org/BeforeSurveyPage ?

Also, the PR should probably point to DEV, right?

[Shnoulle]

I am proposing to add a plugin event to be able to change the survey template.

Why not https://manual.limesurvey.org/BeforeSurveyPage ?

You can not update Template and TemplateConfiguration here, if i don't make error.

[gabrieljenik]

I am proposing to add a plugin event to be able to change the survey template.

Why not https://manual.limesurvey.org/BeforeSurveyPage ?

You can not update Template and TemplateConfiguration here, if i don't make error.

OK, can you expand on the usage of the new event ?

[TonisOrmisson]

Gabriel asks a very good question :) if I look at the beforeSurveyPage event and its description, this looks like a suitable way of changing the template.

But the again testing this I fail to change the template via that event. And from @Shnoulle's comment you think we should not be able to do it from here??

In case the beforeSurveyPage would actually allow changing the template, I would happily use that instead of making another event.

To be honest, the beforeSurveyPage looks like a more suitable way if it would do the job.

@Shnoulle is it not a bug that the template is not actually changed if changed in this event? I myself get a bit lost following why its not actually changing the template from here.

[Shnoulle]

@Shnoulle is it not a bug that the template is not actually changed if changed in this event? I myself get a bit lost following why its not actually changing the template from here.

Seem's your both right : my error : you can change template in BefporeSureyPage.

Remind you change only Survey->template

[Shnoulle]

Agin : i think the BIG issue to fix are ththiss one

Just a note, when logging the amount of afterFindSurvey events on survey page loads, depending on the page there is 5-40+ afterFindSurvey events fired on one page load.

against

Seems like ~5-6 afterFindSurvey calls per questions rendered. Having 10 questions on a page calls afterFindSurvey 60+ times on one page render.

afterFindSurvey must happen ONE time by page load.

This bug is reported ?

[TonisOrmisson]

afterFindSurvey must happen ONE time by page load.

This bug is reported ?

no, not by me, since it seems to be more like a "feature" to me of yii

[TonisOrmisson]

Seem's your both right : my error : you can change template in BeforeSureyPage.

if I change the template name via BeforeSureyPage plugin event then the actual rendered template is not changing, I am guessing it must be a bug then? Or am I missing something? if this is a bug, and replacing the template in BeforeSureyPage event should change the rendered template, then this PR is not needed for the use case I intended.

[TonisOrmisson]

no, not by me, since it seems to be more like a "feature" to me of yii

I would remove the link from LS to Yii's onAfterFind event here entirely to remove the triggering of this event via relations. But again, it might be a feature someone uses, might be "expected" behavior in some cases or some plugins? If you think this seems like a reasonable thing to do, I can report as well as make a PR on this, but it seems like something that needs a bit more eyes to analyze the implications of that.

edit:: and attach the event directly somewhere here

[Shnoulle]

Seem's your both right : my error : you can change template in BeforeSureyPage.

if I change the template name via BeforeSureyPage plugin event then the actual rendered template is not changing, I am guessing it must be a bug then? Or am I missing something? if this is a bug, and replacing the template in BeforeSureyPage event should change the rendered template, then this PR is not needed for the use case I intended.

If you can not update template (to an existing one) : I think it's an issue in 6.

But still : you can update template, but not template/theme option/configuration. Have a new beforeGetTemplate (por aftergetTemplate ?) can be a great idea for some solution.

edit:: and attach the event directly somewhere here

Can you create a DEV mantis issue ?

[TonisOrmisson]

Can you create a DEV mantis issue ?

sure I can! Alhough, I had a test on it and it seems if we only call the afterFindSurvey only once, there seems to be a bunch of things that get broken due to survey setOptions related things and I already got lost there ...

[Shnoulle]

Can you create a DEV mantis issue ?

sure I can! Alhough, I had a test on it and it seems if we only call the afterFindSurvey only once, there seems to be a bunch of things that get broken due to survey setOptions related things and I already got lost there ...

Oh, yes …

Unsure it works good here too … but in my opinion : if value set by plugin setOptions must update only if it's not inherit.

[TonisOrmisson]

https://bugs.limesurvey.org/view.php?id=19555

will do a PR tomorrow ...

[gabrieljenik]

If you can not update template (to an existing one) : I think it's an issue in 6.

Agree. You should be able to replace the temaplte being used.

I would remove the link from LS to Yii's onAfterFind event here entirely to remove the triggering of this event via relations.

Not sure I agree. The afterFind works like an "afterInstantiate" so if it is triggered because of relations then is fine.
I would review what is being implemented on that event and then try to move it to a more approriate event.

Maybe we could have something like a late binding part of the instance that is triggered when needed.

But still, what I say is that having lot of calls to afterFind is not the problem, that is just a sign of of a problem on how the event is being used

[TonisOrmisson]

@gabrieljenik I had similar doubts and also. This is why I was referring to this possibly being a feature, not a bug. That being said, looking at the manual for afterFindSurvey the event is stated to be for:

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

The "current display session" I do not regard as a session for one sub-question context as in $subQuestion->survey. I rather expect the "current display session" to define the whole page view. Currently if I look at the definition of the event purpose to serve "current display session", it looks like a bug. If you ignore the stated purpose it could be regarded as a feature. If one uses it for modifying the current page "display session" it most often is triggered too much.

Anyway, since it is a small change code-wise I will post a PR and we can have a further discussion there :)

[gabrieljenik]

That being said, looking at the manual for afterFindSurvey the event is stated to be for:

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

I think that description is probably outdated.
Look at this:

Possible output
All model attributes (New in 3.22.29 ) can be updated, no control are done.

So overrding display parameters is just one of the usages.

As per the code introduced on this PR, do you think it is still necesarry or the beforeSurveyPage fits your needs?

[TonisOrmisson]

'beforeSurveyPage' should be perfect for me, but last I checked the event failed to change the template.

For me personally a fixed version of 'beforeSurveyPage' would be fine without merging this. Possibly this PR could be usable to someone else, but it does not seem reasonable to merge without specific interest by someone.

[gabrieljenik]

'beforeSurveyPage' should be perfect for me, but last I checked the event failed to change the template.

Maybe that's the bug to fix instead of creating a new feature... I know it worked in the past...Have'nt used it in a while.

[TonisOrmisson]

I have created a bug report in mantis for:
#19572: beforeSurveyPage event is not changing the display template

I have had a look into that but unfortunately I get lost in the numerous plugin instance calls there and I am not able to dig myself into fixing that, hopefully someone more familiar with this functionality in code can fix that.

[TonisOrmisson]

Closing with the hope of a future bug fix for #19572