fix(DeclarativeSettings): Make declarative settings delegation aware#61864
fix(DeclarativeSettings): Make declarative settings delegation aware#61864marcelklehr wants to merge 1 commit into
Conversation
Assisted-by: ClaudeCode:claude-opus-4-8
marcelklehr
requested review from
ArtificialOwl and
salmart-dev
and removed request for
a team
come-nc
left a comment
come-nc
left a comment
There was a problem hiding this comment.
I think this gives too much permissions because it’s based on section id while delegation is for each settings class.
| private function getSectionId(string $app, string $fieldId): string { | ||
| if (array_key_exists($app, $this->appSchemas)) { | ||
| foreach ($this->appSchemas[$app] as $schema) { | ||
| foreach ($schema['fields'] as $field) { | ||
| if ($field['id'] == $fieldId) { | ||
| return $schema['section_id']; | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| throw new Exception('Unknown fieldId "' . $fieldId . '"'); | ||
| } |
There was a problem hiding this comment.
This should be avoided, it’s a net loss in performance.
For getValue which is private we can add the section id to the parameters.
For setValue it may be more complicated but it’s worth trying to avoid it as well if possible. (notably if user is an admin no need to search for section id)
| * or an admin delegation covering the section: delegation is section-based, so | ||
| * being authorized for any (non-declarative) setting of the section also grants | ||
| * access to the declarative forms of that same section. |
There was a problem hiding this comment.
Delegation is not section-based, it’s for each settings class.
| /** @var ISettingsManager|MockObject */ | ||
| private $settingsManager; | ||
|
|
There was a problem hiding this comment.
| /** @var ISettingsManager|MockObject */ | |
| private $settingsManager; | |
| private ISettingsManager&MockObject $settingsManager; | |
|
Ideally a PR for this should add delegation for a declarative settings class to give an example to test it out. |
|
Alright, this was just a one-shot. I don't have much more time I can spend on this. I'm happy to leave this open if somebody wants to tackle this. |
3 participants
Checklist
3. to review, feature component)stable32)AI (if applicable)