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.

Sign up for GitHub

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

Jump to bottom

Declarative settings #42661

Merged
merged 3 commits into from Mar 12, 2024
Merged

Declarative settings #42661

merged 3 commits into from Mar 12, 2024

Conversation

andrey18106
Copy link
Contributor

@andrey18106 andrey18106 commented Jan 9, 2024 •
edited

Summary

This PR introduces Declarative settings implementation that allows PHP apps and ExApps (AppAPI) to register simple schema of settings that will be rendered by server. Settings storage could be handled either by internal storage (as config app/user value), or implemented by apps using provided events:

  1. DeclarativeSettingsGetValueEvent
  2. DeclarativeSettingsSetValueEvent

To register schema apps can use either the IRegistrationContext->registerDeclarativeSettings method to pass their schema (IDeclarativeSettingsForm) or listen to DeclarativeSettingsRegisterFormEvent and pass their schema there. The event is used by AppAPI (the new app ecosystem) because it is more natural than having to dynamically provide IDeclarativeSettingsForm implementations.

All supported fields displayed on screenshots below and available in testing (QA testing) app. These form types declared in OCP\Settings\DeclarativeSettingsTypes.

Screen Shot 2024-01-11 at 14 25 45
Screen Shot 2024-01-11 at 14 26 01
Screen Shot 2024-01-11 at 14 26 20

TODO

  • ...

Checklist

@andrey18106 andrey18106 added this to the Nextcloud 29 milestone Jan 9, 2024
julien-nc
julien-nc requested changes Jan 11, 2024
View reviewed changes
Copy link
Member

@julien-nc julien-nc left a comment •
edited by andrey18106

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • We could revert all changes made in the provisioning_api app
  • The horizontal separator between fields can be removed IMO. A separator between setting sections is enough
  • Maybe we should use label instead of label-inside for NcInputField. To be discussed
  • Field values are not validated yet, should we introduce the regexp attribute now?
  • Fields should not be 100% wide. Let's try: as wide as possible, max 400px
  • generateUrl does not work anymore in this branch, let's rebase on master. We need to remove the changes to compiled assets for an easier rebase
  • For multi select, the label could be placed above and the hint below
  • Same for single checkbox

github-advanced-security[bot]
github-advanced-security bot found potential problems Jan 11, 2024
View reviewed changes
apps/testing/lib/Listener/RegisterDeclarativeSettingsListener.php Fixed Show fixed Hide fixed
apps/testing/lib/Settings/DeclarativeSettingsForm.php Fixed Show fixed Hide fixed
apps/testing/lib/Settings/DeclarativeSettingsForm.php Fixed Show fixed Hide fixed
lib/private/Settings/DeclarativeManager.php Fixed Show fixed Hide fixed
lib/private/Settings/DeclarativeManager.php Fixed Show fixed Hide fixed
apps/settings/lib/Controller/CommonSettingsTrait.php Fixed Show resolved Hide resolved
apps/settings/lib/Controller/CommonSettingsTrait.php Fixed Show fixed Hide fixed
apps/settings/lib/Controller/DeclarativeSettingsController.php Fixed Show fixed Hide fixed
apps/testing/lib/Listener/GetDeclarativeSettingsValueListener.php Fixed Show fixed Hide fixed
apps/testing/lib/Listener/SetDeclarativeSettingsValueListener.php Fixed Show fixed Hide fixed
@andrey18106 andrey18106 marked this pull request as ready for review January 16, 2024 13:14
@andrey18106 andrey18106 force-pushed the feat/settings/declarative branch 2 times, most recently from 35971b7 to 57cead3 Compare February 12, 2024 13:30
@andrey18106 andrey18106 added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Feb 12, 2024
@andrey18106
Copy link
Contributor Author

/compile

@andrey18106 andrey18106 force-pushed the feat/settings/declarative branch 3 times, most recently from 136a5e1 to d72e31d Compare February 12, 2024 20:51
@andrey18106 andrey18106 mentioned this pull request Feb 13, 2024
Merged
github-advanced-security[bot]
github-advanced-security bot found potential problems Mar 7, 2024
View reviewed changes
Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Psalm found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

@julien-nc julien-nc self-requested a review March 7, 2024 13:32
@andrey18106
Copy link
Contributor Author

/compile

@julien-nc julien-nc force-pushed the feat/settings/declarative branch 2 times, most recently from 863f6a6 to be7880f Compare March 8, 2024 11:46
@ChristophWurst
Copy link
Member

Still contains fixups. Mind just squashing this into one commit? It seems the second to last commits are just fixes for the first

@julien-nc julien-nc force-pushed the feat/settings/declarative branch 5 times, most recently from 8f2addf to dcbb10a Compare March 8, 2024 14:26
@julien-nc
Copy link
Member

Psalm failure is unrelated (missing getTeamResourceProviders registration context method).

bigcat88
bigcat88 approved these changes Mar 11, 2024
View reviewed changes
Copy link
Member

@bigcat88 bigcat88 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hope this will be merged in Beta 2..

@Altahrim Altahrim mentioned this pull request Mar 12, 2024
Merged
@provokateurin @andrey18106
feat: Add declarative settings
4ac2375 
Signed-off-by: jld3103 <jld3103yt@gmail.com>
Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
Signed-off-by: Andrey Borysenko <andrey18106x@gmail.com>
@andrey18106
Copy link
Contributor Author

@ChristophWurst @blizzz
rebased PR again,
psalm error unrelated, can we force-merge it?

@andrey18106
Copy link
Contributor Author

/compile

@nextcloud-command
chore(assets): Recompile assets
a7ff09d 
Signed-off-by: nextcloud-command <nextcloud-command@users.noreply.github.com>
@ChristophWurst ChristophWurst added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Mar 12, 2024
@Altahrim
Copy link
Collaborator

Psalm error is related.
Method is deleted in https://github.com/nextcloud/server/pull/42661/files#diff-6f7fce46b3b19b872adcab3426e155b80766fd2b10feabc70ae29cbac2433a40L900 and still used in TeamManager.php

@andrey18106
fix: add missing copyrights and strict types
865fd3b 
Signed-off-by: Andrey Borysenko <andrey18106x@gmail.com>
@Altahrim Altahrim merged commit 92b97ec into master Mar 12, 2024
160 checks passed
@Altahrim Altahrim deleted the feat/settings/declarative branch March 12, 2024 17:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nickvergessen nickvergessen nickvergessen left review comments

@ChristophWurst ChristophWurst ChristophWurst left review comments

@provokateurin provokateurin provokateurin left review comments

@julien-nc julien-nc julien-nc approved these changes

@bigcat88 bigcat88 bigcat88 approved these changes

Assignees
No one assigned
Labels
4. to release Ready to be released and/or waiting for tests to finish developer experience enhancement feature: settings
Projects
None yet
Milestone
Nextcloud 29
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@andrey18106 @ChristophWurst @julien-nc @Altahrim @nickvergessen @bigcat88 @provokateurin @nextcloud-command