N°8528 - ignore silo not applied on 1:n linkedset displayed in form in portal#761
N°8528 - ignore silo not applied on 1:n linkedset displayed in form in portal#761
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes portal linkedset scoping so that the “ignore silos” behavior is correctly applied for 1:n linkedsets displayed in portal forms.
Changes:
- Apply portal scope (via
ScopeValidatorHelper) toAttributeLinkedSet::GetDefaultValue()queries when running in portal context. - Optimize
ormLinkSetindex-building by reducing loaded columns and adjust iteration object loading behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| sources/Core/AttributeDefinition/AttributeLinkedSet.php | Intersects linkedset query with portal scope so silos / ignore-silos rules are applied in portal. |
| core/ormlinkset.class.inc.php | Optimizes column loading during index generation and changes how preserved items are reloaded during iteration. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| { | ||
| $sId = key($this->aPreserved); | ||
| $oRet = MetaModel::GetObject($this->sClass, $sId); | ||
| $oRet = MetaModel::GetObject($this->sClass, $sId, true, true); |
There was a problem hiding this comment.
MetaModel::GetObject($this->sClass, $sId, true, true) sets $bAllowAllData=true, which bypasses user-rights filtering for every ormLinkSet iteration. As ormLinkSet is a core structure used outside the portal, this can expose objects/fields a user is not allowed to read. Instead, pass the AllowAllData flag from the original set/search (eg based on $this->oOriginalSet->GetFilter()->IsAllDataAllowed()), so ignoring silos only happens when the underlying query explicitly allows it.
| $oRet = MetaModel::GetObject($this->sClass, $sId, true, true); | |
| $bAllowAllData = false; | |
| if ($this->oOriginalSet !== null) | |
| { | |
| $bAllowAllData = $this->oOriginalSet->GetFilter()->IsAllDataAllowed(); | |
| } | |
| $oRet = MetaModel::GetObject($this->sClass, $sId, true, $bAllowAllData); |
| { | ||
| $aRet = array(); | ||
| $this->oOriginalSet->Rewind(); | ||
| $this->oOriginalSet->OptimizeColumnLoad([$this->sClass => []]); |
There was a problem hiding this comment.
OptimizeColumnLoad() expects an array keyed by class alias (see DBObjectSet::OptimizeColumnLoad()), but this uses $this->sClass (class name). If the underlying filter alias was renamed (eg to Link in indirect linkedsets), this optimization won’t apply and the set may still load all columns. Use the original filter’s class alias (eg $this->oOriginalSet->GetFilter()->GetClassAlias()) as the key.
| $this->oOriginalSet->OptimizeColumnLoad([$this->sClass => []]); | |
| $oFilter = $this->oOriginalSet->GetFilter(); | |
| $sClassAlias = $oFilter->GetClassAlias(); | |
| $this->oOriginalSet->OptimizeColumnLoad([$sClassAlias => []]); |
There was a problem hiding this comment.
I wonder if there is a way to move this handling inside the portal sources for better isolation 🤔
rquetiez
left a comment
rquetiez
left a comment
There was a problem hiding this comment.
This proposal needs a rework so that the itop core remains uncoupled from external consumers such as the Portal.
| $this->oOriginalSet->Rewind(); | ||
| $this->oOriginalSet->OptimizeColumnLoad([$this->sClass => []]); |
There was a problem hiding this comment.
What is the intention here with regard to the goal of this PR?
There was a problem hiding this comment.
Core implementation MUST NOT be coupled to the portal (here ScopeValidatorHelper, PORTAL_ID)
Could be acceptable solely for maintenance purposes (branch 2.3)
There was a problem hiding this comment.
For what reason this call would be removed ? Fetch can be called in the first place when browsing a set of links.
internal