Conversation
| * | ||
| * @return array<string, mixed> | ||
| */ | ||
| public function prepareInputForUpdate($input): array |
There was a problem hiding this comment.
Data-integrity fix with no non-regression test: nothing asserts that update(['id' => $id, 'entities_id' => $other]) leaves entities_id unchanged while config fields are still updated. tests/Units/ConfigTest.php already has the fixtures needed (createItem(Config::class, ...) / updateItem).
|
|
||
| public function getTabNameForItem(CommonGLPI $item, $withtemplate = 0): string | ||
| { | ||
| if (!self::canView()) { |
There was a problem hiding this comment.
Rights fix with no test: no assertion that getTabNameForItem() returns '' (and displayTabContentForItem() returns false) for a profile without config READ, and returns the tab otherwise.
| $config->update($_POST); | ||
| $config->update( | ||
| ['id' => (int) $_POST['id']] | ||
| + array_intersect_key($_POST, array_flip(Config::getAllConfigFields())), |
There was a problem hiding this comment.
Redundant with the new prepareInputForUpdate(), which applies the same whitelist on every update path. Keeping the filter in one place (the model) is enough; $config->update($_POST) would behave identically and getAllConfigFields() could stay private.
| * | ||
| * @return array<string, mixed> | ||
| */ | ||
| public function prepareInputForUpdate($input): array |
There was a problem hiding this comment.
The whitelist also strips every _-prefixed control key (_no_message, _no_history, keys injected by other plugins' hooks). Harmless today since the only caller is the form, but worth keeping keys starting with _ if internal callers ever update this item.
2 participants
Checklist before requesting a review
Please delete options that are not relevant.