N°7289 - Read-only attribute, dynamically read-write, entry silently ignored on submission#724
N°7289 - Read-only attribute, dynamically read-write, entry silently ignored on submission#724
Conversation
| * this is used to check the current value in GetAttributeFlag function (useful to manage dynamic readonly attributes) | ||
| * @param $sAttr | ||
| */ | ||
| public function GetCurrentValue($sAttCode) |
There was a problem hiding this comment.
Method name too vague and comment are somewhat cryptic to me.
Need some dialog to find how to name and describe things.
| * this is used to check if field has been modifed in GetAttributeFlag function (useful to manage dynamic readonly attributes) | ||
| * @param $sAttr | ||
| */ | ||
| public function IsModifiedValue($sAttCode) |
There was a problem hiding this comment.
Method name too vague and comment are somewhat cryptic to me.
Need some dialog to find how to name and describe things.
5ad63c5 to
d13532d
Compare
Co-authored-by: Thomas Casteleyn <thomas.casteleyn@super-visions.com>
…essage only on console and portal context. Avoid messages during tests, setup and other case
456854d to
88c19e6
Compare
There was a problem hiding this comment.
Pull request overview
Fixes the “dynamic readonly/read-write” behavior during form submission by allowing GetAttributeFlags() logic to rely on posted values (not only persisted ones), so fields that become writable/readonly based on in-form changes are handled correctly.
Changes:
- Add storage of posted form values on
cmdbAbstractObjectand helper methods to read “current value in screen” / detect “modified in screen”. - Populate the stored posted values during
UpdateObjectFromPostedForm()so flag computation can use them. - Adjust when
ormLinkSet::UpdateFromCompleteList()emits a deprecated usage warning.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| core/ormlinkset.class.inc.php | Changes the context gating for the deprecated “old linkedset persistence pattern” warning. |
| application/cmdbabstract.class.inc.php | Stores posted values and adds helpers intended for dynamic GetAttributeFlags() computations during form submission. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * This function checks if the value of the attribute has been modified in screen | ||
| * this is used to check if field has been modified in GetAttributeFlag function (useful to manage dynamic readonly attributes) | ||
| * @param $sAttCode |
There was a problem hiding this comment.
Same as above: the doc references “GetAttributeFlag” (singular / different casing) which doesn’t match the actual GetAttributeFlags() method name. Please fix the wording so integrators know which hook this is intended for.
|
|
||
| $aErrors = array(); | ||
| $aFinalValues = array(); | ||
| $this->aPostedValues = $aValues; // Store the values for later use (e.g. in getAttributeFlag) |
There was a problem hiding this comment.
Minor doc/casing issue: comment says “getAttributeFlag” but the method is GetAttributeFlags() (and this assignment is used before calling it). Please align wording for clarity.
| $this->aPostedValues = $aValues; // Store the values for later use (e.g. in getAttributeFlag) | |
| $this->aPostedValues = $aValues; // Store the values for later use (e.g. in GetAttributeFlags()) |
| if (ContextTag::Check(ContextTag::TAG_PORTAL) || ContextTag::Check(ContextTag::TAG_CONSOLE) ) { | ||
| DeprecatedCallsLog::NotifyDeprecatedPhpMethod('old pattern - please get previous value of the linked set, modify it and set it back to the host object'); |
There was a problem hiding this comment.
The deprecation notification condition changed from “log everywhere except setup” to “log only in portal/console”. This significantly reduces visibility of this deprecated API usage in other contexts (REST/cron/custom scripts) and isn’t mentioned in the PR description; please confirm this behavior change is intended (or restore the previous !TAG_SETUP gating).
| /** | ||
| * @var array Store posted values in order to be used in GetAttributeFlag | ||
| */ | ||
| protected $aPostedValues = []; |
There was a problem hiding this comment.
Indentation of this new property/docblock doesn’t follow the surrounding convention in this file (tabs). Also the doc refers to “GetAttributeFlag” but the actual API is GetAttributeFlags() / GetFormAttributeFlags(); please update the wording to avoid confusion.
| /** | ||
| * function to test if the posted value or if not exists the existing value matches the expected value | ||
| * this is used to check the current value in GetAttributeFlag function (useful to manage dynamic readonly attributes) | ||
| * @param $sAttCode |
There was a problem hiding this comment.
The PHPDoc is misleading: this method doesn’t “test” anything nor compare to an “expected value”; it returns the posted value (if any) or the object’s current value. Please rewrite the doc to match behavior (and reference GetAttributeFlags() with the correct name).
| public function GetCurrentValueInScreen($sAttCode) | ||
| { |
There was a problem hiding this comment.
PR description / example mentions using $this->GetCurrentValue('status'), but the new helper is named GetCurrentValueInScreen(). To make the documented usage work and keep the API intuitive, consider adding a GetCurrentValue() alias (or renaming this method) and updating references accordingly.
use this delta with GetCurrentValue in place of Get and Vincent's example (in bug 7289) will work :