Modernize RoleForm to ipl based RepositoryForm - #5550
Open
jrauh01 wants to merge 12 commits into
Open
Conversation
jrauh01
marked this pull request as draft
July 17, 2026 12:41
lippserd
force-pushed
the
modernize-repository-form
branch
from
July 21, 2026 07:34
4137360 to
133a75e
Compare
jrauh01
force-pushed
the
modernize-repository-form
branch
5 times, most recently
from
July 27, 2026 14:32
0dc7389 to
ef94b62
Compare
jrauh01
force-pushed
the
modernize-role-form
branch
from
July 28, 2026 11:25
04664a1 to
c6a7e7e
Compare
jrauh01
marked this pull request as ready for review
July 28, 2026 11:25
jrauh01
force-pushed
the
modernize-role-form
branch
from
July 30, 2026 15:11
c6a7e7e to
47879ef
Compare
`getPopulatedValue()` returns the raw submitted value, so each check had to know that a checked box submits `'y'`. `CheckboxElement::isChecked()` reports the element's own state instead, which also covers boxes checked through the `checked` option rather than by populated data. The grant checkbox is therefore evaluated after it has been created, as its state is only available once the element exists.
The fieldset used to be a child of the `details` element, which left the module heading in `summary` outside the group its controls belong to. With the order swapped, the entire block including the summary is part of one fieldset. The permission and restriction elements now render inside `details` instead of in the fieldset's own content, so they are created, registered, and decorated explicitly rather than through `addElement()`. The grant checkbox is decorated only once the `DenyToggle` decorator has been attached, otherwise the deny toggle would not be rendered.
In delete mode the form only assembles the submit button, so none of the module fieldsets exist. Transforming their values regardless failed on the missing fieldset keys, which broke removing a role for every setup that provides restrictions.
The hidden element only exists because the visible checkbox is renamed, so both belong to the same branch. Keeping them together drops the second `$hasFullPerm || $hasAdminPerm` check and reuses the already converted element name.
The inherited contract permits `Stringable` and `int`, neither of which this form's constructor accepts. Narrowing the return type documents that and spares `createFilter()` and `onSuccess()` from handling values that cannot occur.
`RoleForm` asks the module manager which privileges to offer, which would make the assembled form depend on the modules installed where the tests run. The test form provides a fixed set instead. Values are populated before the form is assembled, just as when handling a request. Element names and states are derived from what has been submitted, so assembling first would yield a different form.
Moving the form to fieldsets changed how element names map to the privilege names stored in `roles.ini`. `getValues()` carries that mapping alone, so a mistake there silently writes a role granting something else than configured. Empty values are pinned as well. An empty string ends up in the config as a directive, whereas null is skipped.
`fetchEntry()` is the inverse of `getValues()`. Both directions have to agree on the element names, or editing a role silently drops or grants privileges. Every role handed to the repository needs a `parent` property. `collectRoles()` filters on it and `FilterExpression::matches()` reads it off the raw row, which warns or, once Icinga's error handler is in charge, raises an exception the filter swallows.
Reading and writing a role are covered on their own, but only together do they answer what matters: opening a role and saving it again has to leave the configuration alone. Legacy roles are the exception, they are rewritten.
Which checkboxes are disabled, renamed and ignored, which values survive in hidden elements, and which icons summarize a collapsed module is decided while assembling. None of it is visible in the values the form returns. The provided privileges now include a permission carrying the deny prefix. It is the only way to reach the branch that skips the deny toggle for a permission which already is a refusal.
Storing a role is inherited behavior, except that roles inheriting from the one being renamed or removed have to follow. A removal clears their reference with null, an empty string would leave a parent without a name behind. Sorting is verified within a single module only. Full module access has no rule of its own against ordinary permissions, so a set spanning modules sorts differently depending on where it started.
jrauh01
force-pushed
the
modernize-role-form
branch
from
August 4, 2026 06:28
47879ef to
efe7b72
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Migrate
RoleFormandRoleControllerfrom the legacy ZendRepositoryFormto aCompatForm-based repository form.Changes
FieldsetElementvalue mapping: privileges are grouped intoFieldsetElements per module. Because ipl stores fieldset values as nested arrays,fetchEntry()andgetValues()map between the flat ini-key names stored inroles.iniand the nested element structure.assembleCommonElements(): the privilege fieldsets are assembled for insert and update mode.isChecked()for checkboxes: replaces manual value comparisons.getValues()no longer maps privileges when the form is used to remove a role.getIdentifier()return type: to?string, ruling outStringableandintvalues the constructor never accepts.Updatableguard: the constructor throws when the repository does not implementUpdatablein delete mode, which is required to clear parent references on child roles.require Icinga/ipl-html#202