Skip to content
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

Fixed issue #18381: PluginSetting type date must be saved as a common datetime string #2753

Merged
merged 33 commits into from
Mar 31, 2023

Conversation

Shnoulle
Copy link
Collaborator

@Shnoulle Shnoulle commented Dec 1, 2022

Dev: use saveformat option
Dev: Can be set by default to "Y-m-d H:i" ?
Dev: send array of settings, check if datetime and datetimesaveformat is set in array

… datetime string

Dev: use saveformat option
Dev: Can be set by default to "Y-m-d H:i" ?
Dev: send array of settings, check if datetime and datetimesaveformat is set in array
@Shnoulle
Copy link
Collaborator Author

Shnoulle commented Dec 1, 2022

About the last CS : i'm OK to rewrite this part when BOTH commit related was merged  …

#2743 : touch same part …

@olleharstedt
Copy link
Collaborator

Small conflict that needs to be fixed here.

# Conflicts:
#	application/extensions/SettingsWidget/SettingsWidget.php
@Shnoulle
Copy link
Collaborator Author

Small conflict that needs to be fixed here.

Done

@Shnoulle
Copy link
Collaborator Author

There still the decision about default : set format or not ?

@sonarcloud
Copy link

sonarcloud bot commented Jan 30, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@Shnoulle
Copy link
Collaborator Author

Shnoulle commented Mar 2, 2023

@Shnoulle
Copy link
Collaborator Author

Shnoulle commented Mar 2, 2023

There still the decision about default : set format or not ?

@olleharstedt and @maziminke any advice here ? My opinion : default is set to 'Y-m-d H:i:s' but before the fix : default is not set : save as User language preference. I think nobody can use a plugin that save at unpredictable format …

Did we set default or leave at User language preference.

@gabrieljenik
Copy link
Collaborator

gabrieljenik commented Mar 2, 2023

There still the decision about default : set format or not ?

Did we set default or leave at User language preference.

I mentioned here:
#2753 (comment)

But I say it here again as to unify conversations:
Default option seems will not be backward compatible.
The whole idea was that this enhancement was optional and hence bacward compatible.
Is it?

@Shnoulle
Copy link
Collaborator Author

Shnoulle commented Mar 2, 2023

The whole idea was that this enhancement was optional and hence bacward compatible.

backward VS unpredictable

Let's go for unpredictable :)

@Shnoulle
Copy link
Collaborator Author

Shnoulle commented Mar 6, 2023

@gabrieljenik : finally i use PluginsValue , OK ?

On i review all system using only $plugin->settings , i add a Demo plugins showing way to set.

I think we don't need new test since tester is done (… and finally i setup my phpunit instance …)

@gabrieljenik
Copy link
Collaborator

@gabrieljenik : finally i use PluginsValue , OK ?

For me the best and most descriptive is valuesToBeTested. If not, we can always add that explanation as a comment.

I think we don't need new test since tester is done (… and finally i setup my phpunit instance …)

And also you tested the demo plugin :)

application/libraries/PluginManager/PluginBase.php Outdated Show resolved Hide resolved
plugins/Demo/DateSetting/DateSetting.php Outdated Show resolved Hide resolved
plugins/Demo/DateSetting/DateSetting.php Outdated Show resolved Hide resolved
plugins/Demo/DateSetting/DateSetting.php Outdated Show resolved Hide resolved
@Shnoulle
Copy link
Collaborator Author

Shnoulle commented Mar 6, 2023

All done :)

@Shnoulle
Copy link
Collaborator Author

Shnoulle commented Mar 7, 2023

I refuse to fix [Define a constant instead of duplicating this literal "<code>" 8 times.](https://sonarcloud.io/project/issues)

@Shnoulle
Copy link
Collaborator Author

Shnoulle commented Mar 7, 2023

I think here we could call "LimesurveyApi::getFormattedDateTime()" instead of manually doing the conversion.

No : see the last phpDoc , we use getDateFormatData : then need the integer for the orginal format

We have only the f date format, and can be anything "Y" or "H:i" or just "m" etc …

@Shnoulle Shnoulle added Code review done Version checked for code issue without testing and removed Needs code review labels Mar 7, 2023
@Shnoulle
Copy link
Collaborator Author

Great conflict … 😡

Shnoulle and others added 2 commits March 17, 2023 15:15
# Conflicts:
#	tests/data/plugins/SettingsPlugin.php
#	tests/functional/backend/SettingsPluginTest.php
@sonarcloud
Copy link

sonarcloud bot commented Mar 20, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 13 Code Smells

0.0% 0.0% Coverage
11.3% 11.3% Duplication

@olleharstedt olleharstedt merged commit aef1090 into master Mar 31, 2023
@c-schmitz c-schmitz deleted the bug/18381_pluginsetting_date_saveformat branch June 20, 2023 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Code review done Version checked for code issue without testing Has auto test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants