Uh oh!
There was an error while loading. Please reload this page.
feat(lexicon): configurable presets - #53449
Conversation
nickvergessen
left a comment
There was a problem hiding this comment.
Moving my technical comment to the Draft PR, as the issue is more focused on the "content".
I'm not a fan of the approach proposed here and in #51804 (comment) as it's very focused on server and shipped apps.
We need to have the modes given as a constant and should then have a way for each config in it's Lexicon to have a different fall back, so this can be properly done by non-shipped apps as well.
The nested PHP array in a simple file is way to dangerous to have a typo or something sneak in.
So from my POV the "presets" should be handled on each individual entry of the ConfigLexiconEntry with an optional defaultPresetArray array parameter that has the preset name as key and the respective default as a value.
This also allows apps to properly define the config keys within their app without having to wait for a server release, etc. which is very important for apps being released independently or spanning over multiple different server versions.
ArtificialOwl
commented
Jul 9, 2025
I really like this idea, but I would like to advocate for both solution. with a priority on the PHP array.
We could run a check on request (when a new preset is selected, background job, via occ command)
This or a new specific method in
We can assume that a change of value type means a change of the key name. |
nickvergessen
commented
Jul 10, 2025
This is/was not the main idea of the presets. If you want to share a subset of your configuration you can already do that by simply creating a JSON and importing it with
This is and should be controlled by server, so the preset from app's side don't need to handle this.
Default could be a callback that receives the preset?
We are not speaking about config type changes, but let's say Talk introduced a new config in the Summit release first week of June (and we actually had 4), we would need to require people to update server first/as well, before the matching defaults would be available. The main goal is to assist admins with a good experience and developing an own preset is not one of them. It's more important that apps can provide good/sane defaults for known cases out of the box, without having to take loops through server releases. |
2294ff1 to
8e1cf04Compare@nickvergessen : I changed the approach Preset are an Enom and the syntax will use its own method:
First argument can be an array of
|
Uh oh!
There was an error while loading. Please reload this page.
ArtificialOwl
commented
Jul 11, 2025
@nickvergessen beside the dot ( |
8e1cf04 to
c4284a4Compare6832d54 to
cd054ccCompare
nickvergessen
left a comment
There was a problem hiding this comment.
Yeah it goes into a better direction now. Can improve the coding experience a bit more :)
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
1e99307 to
859f630Compare859f630 to
78fa1a4Compare@nickvergessen what about having a second parameter as reference in the Closure that store the returned value in database if set to instead of using |
nickvergessen
commented
Jul 14, 2025
Hmm not sure, I see how it could save calculation efforts, but increases complexity again. All arrays don't have a default that'd depend on preset, most are empty by default. |
bd87605 to
937bb76Compare
nickvergessen
left a comment
There was a problem hiding this comment.
Some comments about the command, otherwise it looks good to go
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
f3623e8 to
d453c9aCompareSigned-off-by: Maxence Lange <maxence@artificial-owl.com>
d453c9a to
e64be71CompareArtificialOwl
commented
Jul 15, 2025
can you please remove your change request ? ;-) |
Required for: #51804
status: review ready.
$defaultRawparameter fromConfigLexiconEntryto set default value based on selectedPresetinconfig.phpusing'config_preset'./occ config:preset [<new_preset>]