-
Notifications
You must be signed in to change notification settings - Fork 988
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
New feature: added beforeGetTemplate plugin event #3816
New feature: added beforeGetTemplate plugin event #3816
Conversation
Just a note, when logging the amount of If one adds a plugin doing anything on that event, the amount of possibly meaningless processing can be huge. |
Seems like ~5-6 |
What ? This must be fixed : Survey::findByPk must call LimeSurvey/application/models/Survey.php Line 1005 in 9b6a908
I think the issue is here before ? No ? Maybe report it separately ? |
$event->set('template', $sTemplateName); | ||
$event = App()->getPluginManager()->dispatchEvent($event); | ||
$sTemplateName = $event->get('template'); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) |
Need a mantis issue please |
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 Nevertheless, it seems unreasonable that switching a template use case is only workable via @Shnoulle you mean mantis issue regarding what here, the 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? |
Good catch ! The original issue are maybe here !
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).
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). |
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 I must say I don not have a strong feeling which suits better, I can refactor that for the |
Yes : its' still a good idea
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 |
Why not https://manual.limesurvey.org/BeforeSurveyPage ? Also, the PR should probably point to DEV, right? |
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 ? |
Gabriel asks a very good question :) if I look at the 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 To be honest, the @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 |
Agin : i think the BIG issue to fix are ththiss one
against
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 |
if I change the template name via |
I would remove the link from LS to Yii's edit:: and attach the event directly somewhere here |
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.
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 |
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. |
https://bugs.limesurvey.org/view.php?id=19555 will do a PR tomorrow ... |
Agree. You should be able to replace the temaplte being used.
Not sure I agree. The afterFind works like an "afterInstantiate" so if it is triggered because of relations then is fine. 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 |
@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:
The "current display session" I do not regard as a session for one sub-question context as in Anyway, since it is a small change code-wise I will post a PR and we can have a further discussion there :) |
I think that description is probably outdated.
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? |
'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. |
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. |
I have created a bug report in mantis for: 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. |
Closing with the hope of a future bug fix for #19572 |
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 muchSurvey::findByPk($sid)
calls, usingafterFindSurvey
event for virtually any type of plugin manipulation seems to generate a huge overhead.Discussion is very welcome :)