View Issue Details
ID | Project | Category | View Status | Date Submitted | Last Update |
---|---|---|---|---|---|
18204 | Feature requests | Plugins | public | 2022-06-21 13:10 | 2022-09-15 11:21 |
Reporter | ollehar | Assigned To | ollehar | ||
Priority | none | Severity | feature | ||
Status | closed | Resolution | fixed | ||
Summary | 18204: New event: ExtendForm | ||||
Description |
| ||||
Tags | No tags attached. | ||||
Bug heat | 6 | ||||
Story point estimate | |||||
Users affected % | |||||
Ping @DenisChenu and @gabrieljenik if you're interested. We will work on this internally and initially only support global settings form and input checkbox. Added to LS5 LTS, I guess (dev branch). |
|
You mean : for GroupSettings ? UserSettings ? near SurveySettings Something related to an object with an id ? https://manual.limesurvey.org/BeforeSurveySettings |
|
This would be a general event for any form that supports it. So, one form event to rule them all. :) |
|
But : you need to review all existing form then ? Before : need to review core form …
|
|
Yeah, but we're just gonna support global settings for now. Rest are added when needed by a plugin author. |
|
|
|
:( To add 3elements
I never have to update global settings , See how i do to create forum since some years now (and it work without update plugin …) : https://gitlab.com/SondagesPro/mailing/sendMailCron/-/blob/master/sendMailCron.php#L1403 We already hae 2 form constructor : one for Survey and one for QuestionAttribute. |
|
No no. |
|
Let me read those again, then. But I doubt they are really flexible, nor object oriented... |
|
Not really flexible … right. |
|
OK, so how would you add a new select2 or datepicker as a survey setting...? |
|
Ah, it's using the SettingsWidget. Also used by plugin settings, right? I thought about that one... |
|
Like always … «je me débrouille comme je peux» example : https://gitlab.com/SondagesPro/OrcidAuthenticate/-/blob/master/views/admin/IntroductionSetting.php |
|
So you'd prefer |
|
I prefer something easy and stable ;) I think i already create a mantis issue about moving QuestoionAttributes + CoreForms + PluginSettings to same extension . Else : i thionk have a definition array without need to create new object each time still instresting |
|
Arrays require more writing than objects, tho. And they have no validation, or defaults. But a middle-ground is to have objects with $input->toSettingsWidget() function that converts them. Idk. |
|
?
core can offer some … PluginSettings have htmlOptions and current
default saveMethod by core can be great |
|
Idk, the settings widget is doing a lot of shit... Starting form tags, registering css and js files, ... It should have been an object hierarchy rather, SettingsWidgetButton, SettingsWidgetBoolean, etc, instead of a function for every setting type. Then it could have been reused in different contexts. |
|
Yes … |
|
Ya, but not sure how... xD Needs info about database table and column and validation. Basically model data.
?? |
|
Urgh. No. |
|
Why not use PluginSettings table by default ? When i mean default : a default table + way to save. I already use PluginSettings table to save User related data. |
|
Then seem:s best to move (after) PluginSettings to the new ExtendForm system, no ? |
|
We also have AdvancedSettingWidget and GeneralOptionWidget for question editor. Already based on DTOs. |
|
Yes, PS : AdvancedSettingWidget are for QuestionAttribute |
|
Hmmm, yes, this works, true. |
|
Lack of optionnal htmlOptions (html5) and way to extend |
|
What's HTML5 html options? Extend how? Something else than new attributes? |
|
Oups … |
|
Actually, does this even need to be an event? We can have a service provider or module instead, where you can add input elements during system init. So.
And then application code just fetch inputs from that service locator when needed. |
|
Need to be in API then , not in App only. Then it's clearly set it can be used without issue since API version are not updated. |
|
Hm, yes, correct. |
|
Would you prefer to have two new events instead, Denis? BeforeGlobalSettings - to add new global setting |
|
Unsure … See wiith @gabrieljenik |
|
Yes, I've pinged Gabriel on Cliq too. |
|
I think I caught the main ideas. Some issues/discussions I believe I noticed: Event for each controlI think this will be too complex, either for implementing from the plugin side, as well as for the plgin invocation side. Also, the saving event needs to be one. We can't have each control with its own saving rutine. (If I understood correctly). Validation EventI believe a validation event is missing and an error message flow could be missing. Other eventsI would also allow a place to add JS and CSS when the form is shown. Control SetupI would reuse what we have in the plugin settings. It could be important to be able to set the position of the extra controls. AfterCloseFormWould be great to be able to have an event like that to inject extra HTML CommentsCan we reuse something from WP hooks maybe? Also, on the plugin setup, maybe we can have something for enabling and disabling for specific forms? We could have the plugin to setup the events by providing an object of a specific class? |
|
Hm, yeah, maybe I can try to write a short spec in the manual, including some example code. Good point about validation. I guess the input objects could throw an exception, like Scripts could also be added as objects, but classes like I wanted to avoid injecting raw HTML. But I guess we can do a I was thinking of trying to re-use the plugin settings widget, if possible. Would have to look more closely. Some more conceptual example code:
Then in the global settings view file, you'd have something like
And in the controller save action:
Hm, maybe And also, maybe |
|
I was thinking something a but more similar to how Yii validations work.
By how you wrote it there, seems like you are adding individual fields, as opposed to adding an extension, which has fields, validations, .... Also, how to handle multuple extensions to the same form? |
|
Hm, you're thinking of active record rules? Or the form functionality?
No problem, the formExtensionService module will just loop the inputs and insert a message for each exception it catches. :)
Yeah, the formExtensionService module would be core code, that accepts individual fields from the plugin init() function.
The formExtensionService maps form names to input arrays, like
But we would probably also have to consider tabs. So $formName might be "globalsettings", or like "globalsettings.email_settings", where the word after the dot is the name of the tab. |
|
Some issues:
|
|
Work started on branch feature/18204-extend-form, trying to use the plugin SettingsWidget at first |
|
As you can see, the SettingsWidget HTML is not adapted to global settings form HTML. |
|
Missing:
|
|
And of course each stupid form has some extra CSS classes that must be obeyed... Hm, maybe factor out render-classes? That can be populated from the form name? So form "globalsettings" has render classes for each input type? |
|
Got something basic running now. Used the exampleSettings plugin, with this code in init(): |
|
Here's the renderer class for global settings: https://github.com/LimeSurvey/LimeSurvey/blob/feature/18204-extend-form/application/libraries/FormExtension/Inputs/GlobalSettingsRenderer.php |
|
Seems great !
Ye s: but here : maybe all form need to have same presentation, same css class etc … |
|
Yes, the plugin authors would not have to touch the plugins to make sure the HTML is up to date, then. :) Hm, if save() is taken care of in the plugins, beforeControllerAction(), then it could be enough with this widget snippet to make a form extensible (assuming there's a renderer for that form):
Else you need the save code in the controller too:
|
|
There should be a |
|
The downside of letting the FormExtensionService class add flash messages - in the pic, the example input threw an exception, and now we have two messages - one success and on failing. :/ But this is necessary to make the change as small as possible to add support to customize one of the forms. Hm, I also made applySave() return a bool for success, so that the controller can adapt, if it wants to. |
|
Nice work!!! Please find some comments:
You would need to do a save and a run for every new field?
What about setting default values?
Is there a place where plugin authors can set which renderer to use per field?
Maybe we can show a summary instead of 2 separated flash messages. |
|
Ya, it's not performant. It's designed to have minimal code impact on the core code-base, so that's easy to add support for more forms, with a very small PR.
Default value can be part of the load() function. :) Just return a default value if non is found in database.
That would defeat the purpose. :D The purpose is to make plugin author oblivious to the html, since it might be different in different LS versions, and for different forms.
Maybe, not sure if that would require more changes to the controllers tho. Or standardize it somehow. |
|
Do we really need classes for JS and CSS scripts? That can also be done in beforeControllerAction, right? |
|
Where is that hook called from?
Yes, but it will allow to do easier customizations per field if needed and wanted
We can think of something for that I believe. Standarized. Don't think of that as the reatest challenge |
|
What about validations? |
|
You can't. That's also a thing that causes too much code impact to core code. You can only prevent save + validate in your save() method, per input field. |
|
You have the RawHtml thing for that, I guess. Currently it doesn't try to save, but I can change that, if you want. Or you use beforeActionController to save it. |
|
Here's the hook for saving: https://github.com/LimeSurvey/LimeSurvey/blob/feature/18204-extend-form/application/controllers/admin/globalsettings.php#L408 Here's the hook for rendering: https://github.com/LimeSurvey/LimeSurvey/blob/feature/18204-extend-form/application/views/admin/globalsettings/_general.php#L377 So you only need two small changes to support a new form (but, sadly, also in this case a new GlobalSettingsRenderer class). |
|
@DenisChenu You have any other comments here, regarding different use-cases? As mentioned, it's adapted for small changes, not to add entire new pages or tabs. |
|
Manual entry here: https://manual.limesurvey.org/Plugins_-_advanced#Form_extension_.28New_in_6_.29 |
|
Quick thought: If this approach will be for individual fields being added to specific places... Why not already providing the means for saving it? I mean KeyValue table, saving routine, Loading as well, ... |
|
Flexibility, mostly. The obvious place to store is in plugin settings storage, but maybe some plugin would like to do some logic or who knows? So passing a save-function gives the dev access to custom logic. But yes, we could include default save-functions, sure. :) |
|
Another thing to take into account then would be: We could provide the related object on the context. |
|
Hmmm, what do you mean with form type? The specific form? The related form object? If there is such? |
|
Merged into develop, because I need to backport it to our cloud, but we can still make changes. Dev branch won't be released for 2 months at least. |
|
Yes. Is this a user level form? A survey level form, ... ? |
|
Aha. What's the related use-case? I see your point, but the usefulness is not obvious to me, sorry. :) |
|
This has been merged into develop. Branch deleted. |
|
Date Modified | Username | Field | Change |
---|---|---|---|
2022-06-21 13:10 | ollehar | New Issue | |
2022-06-21 13:10 | ollehar | Status | new => assigned |
2022-06-21 13:10 | ollehar | Assigned To | => ollehar |
2022-06-21 13:11 | ollehar | Note Added: 70423 | |
2022-06-21 13:11 | ollehar | Bug heat | 0 => 2 |
2022-06-21 13:12 | ollehar | Note Edited: 70423 | |
2022-06-21 14:20 | DenisChenu | Note Added: 70424 | |
2022-06-21 14:20 | DenisChenu | Bug heat | 2 => 4 |
2022-06-21 14:30 | ollehar | Note Added: 70425 | |
2022-06-21 14:36 | DenisChenu | Note Added: 70426 | |
2022-06-21 14:37 | ollehar | Note Added: 70427 | |
2022-06-21 14:39 | ollehar | Note Added: 70428 | |
2022-06-21 14:39 | ollehar | Note Edited: 70428 | |
2022-06-21 14:39 | ollehar | Note Edited: 70428 | |
2022-06-21 14:44 | DenisChenu | Note Added: 70429 | |
2022-06-21 14:52 | ollehar | Note Added: 70430 | |
2022-06-21 14:52 | ollehar | Note Added: 70431 | |
2022-06-21 14:54 | DenisChenu | Note Added: 70432 | |
2022-06-21 15:07 | ollehar | Note Added: 70433 | |
2022-06-21 15:10 | ollehar | Note Added: 70434 | |
2022-06-21 15:12 | DenisChenu | Note Added: 70435 | |
2022-06-21 15:12 | ollehar | Note Added: 70436 | |
2022-06-21 15:12 | ollehar | Note Edited: 70436 | |
2022-06-21 15:13 | ollehar | Note Edited: 70436 | |
2022-06-21 15:15 | DenisChenu | Note Added: 70437 | |
2022-06-21 15:18 | ollehar | Note Added: 70438 | |
2022-06-21 15:24 | DenisChenu | Note Added: 70439 | |
2022-06-21 15:25 | ollehar | Note Added: 70440 | |
2022-06-21 15:28 | DenisChenu | Note Added: 70441 | |
2022-06-21 15:30 | ollehar | Note Added: 70442 | |
2022-06-21 15:32 | ollehar | Note Added: 70443 | |
2022-06-21 15:34 | DenisChenu | Note Added: 70444 | |
2022-06-21 15:34 | DenisChenu | Note Added: 70445 | |
2022-06-21 15:35 | ollehar | Note Added: 70446 | |
2022-06-21 15:37 | DenisChenu | Note Added: 70447 | |
2022-06-21 15:37 | ollehar | Note Added: 70448 | |
2022-06-21 15:38 | DenisChenu | Note Added: 70450 | |
2022-06-21 15:41 | ollehar | Note Added: 70451 | |
2022-06-21 15:49 | DenisChenu | Note Added: 70452 | |
2022-06-22 10:17 | ollehar | Note Added: 70477 | |
2022-06-22 10:25 | ollehar | Note Edited: 70477 | |
2022-06-22 10:25 | ollehar | Note Edited: 70477 | |
2022-06-22 10:35 | DenisChenu | Note Added: 70478 | |
2022-06-22 10:37 | ollehar | Note Added: 70479 | |
2022-08-31 12:38 | ollehar | Description Updated | |
2022-08-31 12:38 | ollehar | Steps to Reproduce Updated | |
2022-09-01 11:14 | ollehar | Note Added: 71571 | |
2022-09-01 11:16 | DenisChenu | Note Added: 71572 | |
2022-09-01 11:16 | ollehar | Note Added: 71573 | |
2022-09-01 15:25 | gabrieljenik | Note Added: 71592 | |
2022-09-01 15:25 | gabrieljenik | Bug heat | 4 => 6 |
2022-09-01 15:31 | gabrieljenik | Note Edited: 71592 | |
2022-09-01 15:33 | gabrieljenik | Note Edited: 71592 | |
2022-09-01 16:50 | ollehar | Note Added: 71594 | |
2022-09-01 16:59 | ollehar | Note Edited: 71594 | |
2022-09-01 17:00 | ollehar | Note Edited: 71594 | |
2022-09-01 17:40 | ollehar | Note Edited: 71594 | |
2022-09-01 17:40 | ollehar | Note Edited: 71594 | |
2022-09-01 19:46 | gabrieljenik | Note Added: 71596 | |
2022-09-02 10:53 | ollehar | Note Added: 71597 | |
2022-09-02 10:54 | ollehar | Note Edited: 71597 | |
2022-09-02 11:02 | ollehar | Note Edited: 71597 | |
2022-09-02 11:03 | ollehar | Note Edited: 71597 | |
2022-09-06 18:12 | ollehar | Note Added: 71611 | |
2022-09-06 18:12 | ollehar | Note Added: 71612 | |
2022-09-06 18:16 | ollehar | Note Added: 71613 | |
2022-09-06 18:16 | ollehar | File Added: pic.png | |
2022-09-06 18:29 | ollehar | Note Added: 71614 | |
2022-09-06 18:38 | ollehar | Note Added: 71615 | |
2022-09-06 19:36 | ollehar | Note Added: 71617 | |
2022-09-06 19:38 | ollehar | Note Added: 71618 | |
2022-09-06 19:52 | DenisChenu | Note Added: 71619 | |
2022-09-06 19:55 | DenisChenu | Note Edited: 71619 | |
2022-09-06 19:57 | ollehar | Note Added: 71620 | |
2022-09-06 19:58 | ollehar | Note Added: 71621 | |
2022-09-06 19:59 | ollehar | Note Edited: 71620 | |
2022-09-06 19:59 | ollehar | Note Edited: 71620 | |
2022-09-06 19:59 | ollehar | Note Edited: 71620 | |
2022-09-07 12:35 | ollehar | Note Added: 71625 | |
2022-09-07 12:35 | ollehar | File Added: image.png | |
2022-09-07 12:36 | ollehar | Note Edited: 71625 | |
2022-09-07 12:45 | ollehar | Note Edited: 71625 | |
2022-09-07 13:31 | gabrieljenik | Note Added: 71626 | |
2022-09-07 13:32 | gabrieljenik | Note Edited: 71626 | |
2022-09-07 15:07 | ollehar | Note Added: 71627 | |
2022-09-07 15:07 | ollehar | Description Updated | |
2022-09-07 15:07 | ollehar | Description Updated | |
2022-09-07 15:08 | ollehar | Description Updated | |
2022-09-07 15:17 | ollehar | Note Added: 71628 | |
2022-09-07 16:21 | gabrieljenik | Note Added: 71629 | |
2022-09-07 16:21 | gabrieljenik | Note Added: 71630 | |
2022-09-07 16:24 | ollehar | Note Added: 71631 | |
2022-09-07 16:26 | ollehar | Note Added: 71632 | |
2022-09-07 16:31 | ollehar | Note Added: 71633 | |
2022-09-07 17:44 | ollehar | Note Added: 71634 | |
2022-09-07 18:07 | ollehar | Note Added: 71635 | |
2022-09-08 12:55 | gabrieljenik | Note Added: 71644 | |
2022-09-08 13:06 | ollehar | Note Added: 71645 | |
2022-09-08 13:17 | gabrieljenik | Note Added: 71646 | |
2022-09-08 13:20 | ollehar | Note Added: 71647 | |
2022-09-08 13:22 | ollehar | Note Added: 71648 | |
2022-09-08 13:26 | gabrieljenik | Note Added: 71649 | |
2022-09-08 14:23 | ollehar | Note Added: 71652 | |
2022-09-15 11:21 | ollehar | Status | assigned => closed |
2022-09-15 11:21 | ollehar | Resolution | open => fixed |
2022-09-15 11:21 | ollehar | Note Added: 71758 |