Can you fix this without breaking old plugins?

I don't think this is a bug, but a feature.
Currently there is no typing at all used for saving plugin settings. The existing typing only determines how the setting is displayed using the SettingsWidget.

To keep backward_compatibilty I would just introduce a new field type and name it 'datetime' instead of 'date'. Mark 'date' as obsolete for future major versions.

The fix for https://bugs.limesurvey.org/view.php?id=18330 leads to a far better usage of date type fields at plugin settings. But it can also cause serious issues. A user with date format setting yyyy-mm-dd may be entering a plugin setting like 2022-12-31. The date may be used for certain calculations or triggering email sending.
Then another user e.g. from Germany with date format dd.mm.yyyy edits the same setting and changes the day to "24" but then it gets saved as 24.12.2022 at the DB. All calculations or the like will be broken.

So whatever solution we aim for, we should make sure that we always have a valid date format stored for this field type, similar to how we deal with a token's expiry date for example.

Can you fix this without breaking old plugins?

We have same issue with password : how to fix it …

To keep backward_compatibilty I would just introduce a new field type and name it 'datetime' instead of 'date'. Mark 'date' as obsolete for future major versions.

We don' have different way to save settings according to type currently … we can fix it for surveySetting before send it to the event. But for global one : there are only one function maybe extended by plugin.

Survey : https://github.com/LimeSurvey/LimeSurvey/blob/32aa344f92cee033805f3f788aac2f48a8cfded0/application/controllers/admin/Database.php#L661 We can update some before send it to plugin
Global : https://github.com/LimeSurvey/LimeSurvey/blob/32aa344f92cee033805f3f788aac2f48a8cfded0/application/libraries/PluginManager/PluginBase.php#L257 (i extend it a lot …)

The fix for https://bugs.limesurvey.org/view.php?id=18330 leads to a far better usage of date type fields at plugin settings.

Maybe best solution is to force datetime to be in ISO/SQL format here ?

Forcing ISO/SQL format is fine with me. We just need to make sure that users are not able to enter something invalid.

@c_schmitz, can we assign this to a developer so it gets included at the next sprint? We need it in 2-4 weeks for a new project.

@DenisChenu, do you want to pick this one since you also worked on https://bugs.limesurvey.org/view.php?id=18330 ?

I am sorry but it is not a priority for us and it is more like a new feature. Maybe you can hire Denis directly, if this can be implemented in a backward-comptible manner?

@bismark have competency to do it i think :)

Else : the way i do it for password ? https://github.com/LimeSurvey/LimeSurvey/pull/2651

Adding a option : 'saveasiso'=>true

My preference is adding a new setting type "isodate" which has a date picker but always stores details as YYYY-MM-DD format.

We just need the time for implementing this. @gabrieljenik, would you be able to provide a helping hand (paid support)?

isodate is better than saveasiso :+1:

Currently not a lot of time :)

I think the term "attribute on the setting type" and "setting type" are begin used inter-changabely.

With an attribute on the setting type, the impact is minor I think, right?

With an attribute on the setting type, the impact is minor I think, right?

Extra options 'isodate' => false by default don't change anything for old plugins.

HI @DenisChenu,

From what I spoken with Marcel, you will take this, right?

?

I don't remind , but OK :)

@DenisChenu, is there a certain timeline for this?

Oups , was on feature …

Shit WhDateTimePicker didn't allow adding function in https://bootstrap-datepicker.readthedocs.io/en/latest/events.html#

If this would have been easy to implement, we would not have asked you to look into it ;-)

Shit WhDateTimePicker didn't allow adding function

Is that a frontend based solution?
I thoguht it would be more inclined into a backend handling.

Seems WhDateTimePicker is out of date … no way to get js events working …

https://github.com/LimeSurvey/LimeSurvey/pull/2753

Update plugin

        'checkDate1'=>array(
            'type'=>'date',
            'label' => 'A date, only year and month was saved',
            'default' => '',
            'saveformat' => "Y-m"
        ),
        'checkDate2'=>array(
            'type'=>'date',
            'label' => 'Another date',
            'default' => '',
        ),
        'checkDate3'=>array(
            'type'=>'date',
            'label' => 'A date saved as shown',
            'default' => '',
            'saveformat' => null
        ),

@bismark, I think this implementation makes sense and offer sufficient flexibility to adjust our plugins as needed?

I'm really unhappy to don't find a JS way only …

I try all events in https://bootstrap-datepicker.readthedocs.io/en/latest/events.html

No one seems to work …

@gabrieljenik

Is that a frontend based solution?

All plugin settings are frontend helper only : currently NOTHING are checked or updated by default.
We can not have a "only backend" solution …

See the commit, i need to add hidden in frontend to check it in backend.

We can check setting type for Global Setting
But for NewSurveySettings : we can not add an event BeforeSurveySettings just to have current SurveySettings information.
Then : we have only frontend validation for core.

Plugin must/can do own validation in NewSurveySettings event

We can not have a "only backend" solution …

Why not?

We don't have any information on settings set in beforeSurveySettings
https://github.com/LimeSurvey/LimeSurvey/blob/e54d2f532b363e474f679e6392e59da06efb8000/plugins/Demo/Example/Example.php#L30

PS : adding beforeSurveySettings a second time in Database : you surely broke some plugins …

Converting date here wouldn't work?
https://github.com/LimeSurvey/LimeSurvey/blob/master/application/libraries/PluginManager/PluginBase.php#L275

Oups,
Not https://github.com/LimeSurvey/LimeSurvey/blob/e54d2f532b363e474f679e6392e59da06efb8000/application/libraries/PluginManager/PluginBase.php#L257 but

function set : issue it's same in function set : you don't have information on Survey settings …

function set : issue it's same in function set : you don't have information on Survey settings …

Well, then maybe we should create a new method setForSurvey()?

Current set method
protected function set($key, $data, $model = null, $id = null)

We keep old method and create a new one
protected function setForSurvey($key, $data, $surveyId = null)

Signature would be pretty similar, but when using that we would be already sure that the id parameter (4th one in the signature) belongs to a survey id.

If not, what you did on DB is OK.

When saying this, ...

Is that a frontend based solution?

The conversation got driven to the save process.

I realize we should be reviewing the read process.

Almost all these you did on the SettingsWidget
https://github.com/LimeSurvey/LimeSurvey/pull/2753/files#diff-6bbdf0ee6329bf62fdeb4326696b98f1a3b4879e075f9a9004a37e0d533ec34cR621

Can that be moved to a new getForSurvey() method?
protected function getForSurvey($key, $surveyId = null)

Maybe that's a bit cleaner ?

Well, then maybe we should create a new method setForSurvey()?

The only way to get PHP information on surveySettings are to

1. Call again beforeSureySettings : i think it's a really bad solution
2. Create a new event

I realize we should be reviewing the read process.

Why ? The plugin read with $this->get('setting','survey',$sid) , it's the plugin task to don't break anything with this …

Well, then maybe we should create a new method setForSurvey()?
Can that be moved to a new getForSurvey() method?

Then add again exception ? And after have all getForUser etc … ?

I don't like this …

@gabrieljenik about «All plugin settings are frontend helper only»

you don't seem to trust me on this point? Have you looked, tried?

If you don't trust me on this point: take the bug and create an alternative PR, it will go faster.

you don't seem to trust me on this point? Have you looked, tried?

It is not a matter about trust.
We were discussing and reviewing.

I believe there could be a different approach which I like more.
Still, that doesn't mean better.

If your approach works and my arguments don't convince you, then, good, keep on going.
We don't have to write code the same way :)

Just please also invite Olle or Casten to do a review as their point of view will probably be helpfull.

I believe there could be a different approach which I like more.

I really want a clean solution to have a real PHP control too.
But it's something we have to discuss when PluginSettings was created some years ago.

Now : there are 2 solution : rewriting WHOLE plugin setting process to allow a real PHP control
Or fix such issue using some workaround.

You currently discuss on « rewriting WHOLE plugin setting»…

PS : like i write in commit : https://github.com/LimeSurvey/LimeSurvey/pull/2753#issuecomment-1333844087

I'm OK to rewrite this part when BOTH commit related was merged …

I mean create a new function in PluginBase like $value = $oPlugin->fixSetting(); for example.

But since there are 2 commit currently needing it … i need to have one merged …

You currently discuss on « rewriting WHOLE plugin setting»…

I don't think I am saying rewrite whole plugin system.
Still, this discussion seems to be going in circles.

So, I will just review the code and move over :)

I believe there could be a different approach which I like more.

Tell me clearly the different approach ? Without rewriting PluginSetting system …

There are no back-end solution with the current Plugin setting system, i write it and try to explain … but you ask again why …

There are no back-end solution with the current Plugin setting system, i write it and try to explain … but you ask again why …

Yes, we are trying to get in sync, but can't get to it.
So, no worries. We can move along.

Still, as to try one more time, basically my idea is:

• Convert from current format to YMD format before save (you arleady did on Database
• Convert from YMD (stored in the DB) to expected format right after fetching from the DB,
  You did on the widget. I believe it can be done closer to the plugin,
  Around $this->get('setting','survey',$sid)

But that's an idea. I haven't done a PoC.
If you say it can't be done, let's move over. We don't need both of us spending time on this :)

Thanks!

@ollehar, can we merge this into the next release?

@ollehar, just a friendly reminder...

Hm. Small conflict in the PR now. Wrote a comment.

Done
Test plugin

        'checkDate1'=>array(
            'type'=>'date',
            'label' => 'A date, only year and month was saved',
            'default' => '',
            'saveformat' => "Y-m"
        ),
        'checkDate2'=>array(
            'type'=>'date',
            'label' => 'Another date',
            'default' => '',
        ),
        'checkDate3'=>array(
            'type'=>'date',
            'label' => 'A date saved as shown',
            'default' => '',
            'saveformat' => false
        ),

Here :
checkDate1 always saved as Y-m-0 0:0
checkDate2 always saved as Y-m-d H:i (Default)
checkDate3 saved like before

@bismark, I think this works as needed?

@DeniChenu, thanks for your help with this.

I think it's better to save as Y-m-d H:i:s by default
Unsure it can really broke any existing plugins ?

Denis

I downloaded TestPlugin and tried to save global plugin settings, which results in:
Call to undefined method LimeSurvey\PluginManager\LimesurveyApi::getFixedDateTime()

Oups … sorry for this …

Fixed https://github.com/LimeSurvey/LimeSurvey/pull/2753

@DenisChenu, @ollehar: This was tested fine, can you please merge the adjustment into the next 5.x release?

Do we need to extend the manual for this? Where are such details documented? If you share a link, I can work on the German translation.

Do we need to extend the manual for this? Where are such details documented? If you share a link, I can work on the German translation.

I can not update manual when not merged ;)

After adding it here https://manual.limesurvey.org/Plugins_-_advanced#Plugin_settings ?
You find another place ?

@ollehar @gabrieljenik: Can we finally merge this? 5 months have passed since our bug report and we really need to use this for a customer project.

Denis and Gabriel are paid to review and test each other's work. So, nagging on them is the way forward. :)

@gabrieljenik, @DenisChenu nagging you to get this released ;-)

Assigning it back to Denis as semes some updates are still undergoing.

Yes, sorry …

Have a test + a demo plugin
(last step : update manual)

Can we merge the fix into the next release?

Fix committed to master branch: http://bugs.limesurvey.org/plugin.php?page=Source/view&id=34408

Fixed in Release 5.6.14+230403

Please check again and update the manual

For the manual : there are no Plugin_Settings page … can you create a page ?
We have a demo plugin : https://github.com/LimeSurvey/LimeSurvey/blob/master/plugins/Demo/DemoDateSetting/DemoDateSetting.php

For check again : what do you mean ?

@bismark, please share some of the code you tested, what the outcome of the test and data stored at the DB was and what exact part you assume is not working as expected.

Remind : decision was done to don't set a format by default.

Discussion is here about default : https://github.com/LimeSurvey/LimeSurvey/pull/2753#issuecomment-1451634613

ping @bismark ?

I'm on discord if needed

no feedback