N°9622 - Error in the user story life cycle - #915
Conversation
|
| Filename | Overview |
|---|---|
| application/cmdbabstract.class.inc.php | Updates single-allowed-value defaulting in lifecycle forms, but the linkset handling is incomplete for indirect links and creation forms. |
Comments Outside Diff (1)
-
application/cmdbabstract.class.inc.php, line 3106-3110 (link)The creation-form path still applies the old scalar defaulting logic to linkset attributes. If a mandatory linkset has exactly one allowed value during object creation, this calls
Set()with the scalar id fromarray_keys().DBObject::Set()treats non-null linkset values as an object-set iterator and callsUpdateFromCompleteList($value), so the creation form can fail before the user can submit it. This is the same single-allowed-linkset case handled below for lifecycle transitions, but this sibling path remains unchanged.
Reviews (1): Last reviewed commit: "9622 - Error in the user story life cycl..." | Re-trigger Greptile
| if ($oAttDef->IsLinkSet()) { | ||
| $oObjectSet = $this->Get($sAttCode); | ||
| $oObjectSet->AddItem(MetaModel::GetObject($oAttDef->GetLinkedClass(), $aValues[0])); | ||
| $this->Set($sAttCode, $oObjectSet); |
There was a problem hiding this comment.
Indirect linksets misload objects
For an AttributeLinkedSetIndirect, GetAllowedValues_att() returns ids from the remote target class, because AttributeLinkedSet::GetValuesDef() uses LinkSetModel::GetTargetClass(). This branch then loads that id from $oAttDef->GetLinkedClass(), which is the link/intermediate class. When a lifecycle transition has exactly one allowed remote object, the form can throw because no link row has that id, or it can add an unrelated existing link row with the same numeric id. The indirect case needs to create a link object and set its remote external key instead of loading the link class by the remote id.
There was a problem hiding this comment.
Right, the proposed solution doesn't work with AttributeLinksetIndirect.
There was a problem hiding this comment.
Confirmed. Looking at the current diff, it seems the fix has already been updated to simply skip all linksets (!$oAttDef->IsLinkSet()), which is the safe fallback. That avoids the indirect problem entirely.
However, there are two things still worth noting:
1. Direct linksets are also skipped now — for AttributeLinkedSet (non-indirect), the allowed values are ids of GetLinkedClass(), so the original GetObject call would have been correct there. The current fix under-handles that case too.
2. Leftover debug code — there's dead code introduced at line ~3425:
if ($this->GetContext()['objects_to_insert']) {
$a = $this->GetContext()['objects_to_insert'];
}This does nothing (assigns to $a which is never used) and should be removed before merge.
For a complete fix of the indirect case, the correct pattern would be:
if ($oAttDef->IsIndirect()) {
$oLink = MetaModel::NewObject($oAttDef->GetLinkedClass());
$oLink->Set($oAttDef->GetExtKeyToRemote(), $aValues[0]);
$oObjectSet = $this->Get($sAttCode);
$oObjectSet->AddItem($oLink);
$this->Set($sAttCode, $oObjectSet);
} elseif ($oAttDef->IsLinkSet()) {
$oObjectSet = $this->Get($sAttCode);
$oObjectSet->AddItem(MetaModel::GetObject($oAttDef->GetLinkedClass(), $aValues[0]));
$this->Set($sAttCode, $oObjectSet);
} else {
$this->Set($sAttCode, $aValues[0]);
}Whether to implement that or keep the "skip all linksets" approach depends on whether auto-filling linksets during stimulus transitions is a desired behaviour for your use case.
Tip: You can customize Greptile's behavior for this repo with .greptile/rules.md and .greptile/config.json.
| $this->Set($sAttCode, $aValues[0]); | ||
| if ($oAttDef->IsLinkSet()) { | ||
| $oObjectSet = $this->Get($sAttCode); | ||
| $oObjectSet->AddItem(MetaModel::GetObject($oAttDef->GetLinkedClass(), $aValues[0])); |
There was a problem hiding this comment.
is there any way $aValues contains more than one single value?
can you add a test that cover DisplayStimulusForm with this new fixed usecase (linkset), pls.
There was a problem hiding this comment.
what if GetObject throws an exception when object does not exist? if it could happen, it could be nice for support team to catch the exception, log it and return false (i guess?).
There was a problem hiding this comment.
If the attribute is a LinkSet, GetAllowedValues_att returns an ObjectSet from an OQL query. Therefore, I believe this object must exist.
There was a problem hiding this comment.
And to answer the first question, it is impossible for $Values to contain more than one value, because the test count($allowedValues) == 1 was performed just before.
There was a problem hiding this comment.
ok technically it is not possible. you are right (count==1).
so functionally this PR fixes stimulus form display of current object, by adding first found allowed value to the linkset, right? no all possible allowed values. one is enough...
if there is one value to add, cant it be default one if it exists? maybe it does not make sense?
|
I discussed this fix with @rquetiez, we need to dig deeper into the cause. For now we focus on the 3.3.0-beta, discussion will be done afterwards. |
There was a problem hiding this comment.
Pull request overview
Fixes lifecycle transition errors caused by auto-selecting a sole link-set value.
Changes:
- Excludes link sets from automatic value assignment.
- Adds an ineffective context lookup that can trigger warnings.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- do not perform prefill on stimulus for linksets
fceccb0 to
52c7226
Compare
Internal