From 3a13b74dd33d9ef90058404fc42abc4f8757d7ae Mon Sep 17 00:00:00 2001 From: Marc Jauvin Date: Mon, 6 Jul 2026 09:03:32 -0400 Subject: [PATCH 1/8] add changed to RelationController behavior to support deeply nested relations --- .../backend/behaviors/RelationController.php | 538 ++++++++++- .../partials/_button_delete.php | 4 +- .../partials/_button_remove.php | 4 +- .../partials/_button_unlink.php | 2 +- .../backend/formwidgets/RelationManager.php | 14 + .../RelationControllerNestedTest.php | 842 ++++++++++++++++++ modules/system/assets/ui/js/popup.js | 21 + 7 files changed, 1403 insertions(+), 22 deletions(-) create mode 100644 modules/backend/tests/behaviors/RelationControllerNestedTest.php diff --git a/modules/backend/behaviors/RelationController.php b/modules/backend/behaviors/RelationController.php index ee51977856..477336b313 100644 --- a/modules/backend/behaviors/RelationController.php +++ b/modules/backend/behaviors/RelationController.php @@ -196,6 +196,48 @@ class RelationController extends ControllerBehavior */ protected $foreignId; + /** + * @var string Fully-qualified bracket path identifying the CURRENT + * field at any nesting depth, e.g. "items[5][taxes]" - a relation + * manager rendered inside another relation manager's own manage + * form. Bracket syntax mirrors the array-style naming already used + * for nested fields elsewhere (e.g. `author[name]`, a repeater's + * `repeaterField[0][subfield]`). An id sits between each relation + * name because a hasMany/belongsToMany hop is ambiguous without + * one - "the taxes of item #5" needs to say WHICH item, unlike a + * hasOne/belongsTo hop. Equal to $field for an unnested relation. + */ + protected $nestedField = ''; + + /** + * @var \Winter\Storm\Database\Model The true root model this + * controller/page was booted with, captured once per request. + * Needed to walk a bracket path from the top when a nested field + * is being resolved from a fresh request rather than an ambient, + * same-request render (see resolveNestedContext()). + */ + protected $rootModel; + + /** + * @var array LIFO stack of ambient bracket paths, e.g. ["items[5]"]. + * Pushed in relationMakePartial() right before a manage form/pivot + * partial renders, popped right after. Whatever's on top when a + * nested field's initRelation() runs during that render IS its + * parent context - no parsing needed, since the model handed in is + * already the widget's own bound model. + */ + protected $nestingStack = []; + + /** + * @var array Session key cache, keyed by $nestedField. $sessionKey + * above is a single shared property - whichever field is resolved + * first in a request sets it, and the check in + * relationGetSessionKey() would otherwise hand that SAME stale key + * back to every subsequently-resolved field in the same request, + * rather than generating its own. See relationGetSessionKey(). + */ + protected $sessionKeysByField = []; + /** * @var mixed Configuration for this behaviour */ @@ -240,6 +282,22 @@ public function __construct($controller) */ protected function validateField($field = null) { + // Falling back to post(PARAM_FIELD) unconditionally assumes + // that value unambiguously identifies "the" active field - + // several methods call validateField() this way with no field + // at all (relationRenderToolbar(), relationRenderView(), + // relationRefresh()). Once a field has already been resolved + // this request, prefer it: the raw POST value can still be a + // full bracket path (e.g. "items[15][taxes]", the qualified + // value the request originally targeted) even after + // $this->field has already been narrowed down to the bare leaf + // ("taxes") by an earlier initRelation() call in the same + // request - triggering a redundant, wasteful re-resolution for + // no benefit. + if ($field === null && $this->field) { + $field = $this->field; + } + $field = $field ?: post(self::PARAM_FIELD); if ($field && $field != $this->field) { @@ -262,7 +320,27 @@ public function prepareVars() $this->vars['relationManageId'] = $this->manageId; $this->vars['relationLabel'] = $this->config->label ?: $this->field; $this->vars['relationManageTitle'] = $this->manageTitle; - $this->vars['relationField'] = $this->field; + + // A single value drives the container's own + // `data-request-data="_relation_field: ..."` attribute (which + // every button inherits automatically - Winter's AJAX + // framework merges data-request-data from ancestor elements), + // and the manage form's own hidden `_relation_field` input. + // Using the fully-qualified path for a nested field is enough + // on its own to make both correct, with no separate + // per-mechanism patching needed. + $this->vars['relationField'] = $this->isNestedField() ? $this->nestedField : $this->field; + + // Always the bare field name, even when nested - used + // specifically by $.wn.relationBehavior.changed(...) (the + // Delete/Unlink/Remove button partials), which matches against + // `[data-field-name="..."]` selectors that form field wrappers + // set to the bare name. relationField above can't serve this + // purpose once qualified: a wrapper for a nested field's own + // "taxes" widget has data-field-name="taxes", never the + // qualified path. + $this->vars['relationFieldName'] = $this->field; + $this->vars['relationType'] = $this->relationType; $this->vars['relationSearchWidget'] = $this->searchWidget; $this->vars['relationManageFilterWidget'] = $this->manageFilterWidget; @@ -317,13 +395,18 @@ public function initRelation($model, $field = null) } $this->config = $this->originalConfig; - $this->model = $model; - $this->field = $field; if ($field == null) { + $this->model = $model; + $this->field = $field; return; } + [$model, $field, $isAmbient] = $this->resolveNestedContext($model, $field); + + $this->model = $model; + $this->field = $field; + if (!$this->model) { throw new ApplicationException(Lang::get('backend::lang.relation.missing_model', [ 'class' => get_class($this->controller), @@ -337,6 +420,8 @@ public function initRelation($model, $field = null) ])); } + $this->aliasNestedConfig($field); + if (!$this->getConfig($field)) { throw new ApplicationException(Lang::get('backend::lang.relation.missing_definition', compact('field'))); } @@ -357,8 +442,15 @@ public function initRelation($model, $field = null) $this->relationObject = $this->model->{$field}(); $this->relationModel = $this->relationObject->getRelated(); - $this->manageId = post('manage_id'); - $this->foreignId = post('foreign_id'); + // manage_id/foreign_id are single global POST values with no + // per-field scoping - meaningless (or actively wrong) for a + // nested field in certain situations, since stock's own JS + // keeps one shared hidden input for each regardless of which + // container/field is actually involved. See + // shouldSuppressManageId()/shouldSuppressForeignId() for + // exactly when each is suppressed and why they differ. + $this->manageId = $this->shouldSuppressManageId($isAmbient) ? null : post('manage_id'); + $this->foreignId = $this->shouldSuppressForeignId($isAmbient) ? null : post('foreign_id'); $this->readOnly = $this->getConfig('readOnly'); $this->deferredBinding = $this->getConfig('deferredBinding') || !$this->model->exists; $this->viewMode = $this->evalViewMode(); @@ -418,6 +510,354 @@ public function initRelation($model, $field = null) } } + /** + * @return bool Whether the CURRENT field is nested inside another + * relation manager's own manage form, at any depth. + */ + protected function isNestedField() + { + return strpos($this->nestedField, '[') !== false; + } + + /** + * Resolves $model/$field into the actual model + leaf field name + * initRelation() should operate on, setting $this->nestedField + * (and $this->rootModel, on the first call this request) as a side + * effect. Three cases: + * + * a) AMBIENT - the nesting stack is non-empty, meaning a manage + * form is actively rendering right now (see + * relationMakePartial()) and this field is nested directly + * under whatever's on top of it. $model is trusted as-is - it's + * the widget's own bound model. + * b) RECONSTRUCTED - $field is already a bracket path (e.g. + * "items[5][taxes]"), walked from the root to find the actual + * model. This is the common case for anything other than an + * ambient render: prepareVars() makes the container's + * data-request-data, every button that inherits it, and the + * manage form's own hidden field all carry the fully-qualified + * path for a nested field, so post(PARAM_FIELD) already has + * everything needed here. + * c) ROOT - a plain field, empty stack: unchanged stock behaviour. + * + * If $model is null (the common case for a field that's nested - + * see resolveFallbackRootModel()'s own docblock for why) and no + * fallback can be found either, returns null for the model so the + * caller's own existing "missing model" check reports it exactly + * as it would have before nesting existed. + * + * @return array [$model, $field, $isAmbient] + */ + protected function resolveNestedContext($model, $field) + { + if ($model === null) { + $model = $this->resolveFallbackRootModel(); + } + + if ($model === null) { + return [null, $field, false]; + } + + if ($this->rootModel === null) { + $this->rootModel = $model; + } + + $ambientPath = end($this->nestingStack) ?: null; + + if ($ambientPath !== null) { + $this->nestedField = $ambientPath . '[' . $field . ']'; + return [$model, $field, true]; + } + + if (strpos($field, '[') !== false) { + $this->nestedField = $field; + [$hops, $leafField] = $this->parseBracketField($field); + return [$this->resolveModelForHops($this->rootModel, $hops), $leafField, false]; + } + + $this->nestedField = $field; + return [$model, $field, false]; + } + + /** + * Recovers the root model when initRelation() is invoked (e.g. from + * an AJAX handler like onRelationButtonCreate) without a widget + * having rendered first to prime $this->model. + * + * This is the common case for a NESTED field specifically: a + * root-level relation field is always pre-initialized during the + * controller's own page action (FormController builds the entire + * form widget tree - including every relationmanager-type field + * widget, each of which calls initRelation() itself - before any + * AJAX handler runs), so $this->model is already set by the time a + * handler needs it. A nested field (e.g. "taxes" on an Item) is + * only ever rendered inside a popup that's built separately, later, + * INSIDE the handler itself (relationMakePartial('manage_form') -> + * $this->manageWidget->render()) - it never goes through that + * pre-initialization, so $this->model is genuinely null the first + * time such a field needs resolving. + * + * Tries the conventional FormController accessor. If a controller + * loads its record some other way, this is the one place to adjust. + */ + protected function resolveFallbackRootModel() + { + if (method_exists($this->controller, 'formGetModel')) { + return $this->controller->formGetModel(); + } + + if ($this->controller->isClassExtendedWith(\Backend\Behaviors\FormController::class)) { + return $this->controller->asExtension(\Backend\Behaviors\FormController::class)->formGetModel(); + } + + return null; + } + + /** + * Parse "items[5][taxes][2][adjustments]" into: + * hops: [['items', '5'], ['taxes', '2']] + * leaf: 'adjustments' + */ + protected function parseBracketField($field) + { + preg_match('/^([^\[]+)((?:\[[^\]]*\])+)$/', $field, $matches); + + if (!$matches) { + throw new ApplicationException("Malformed nested relation field: \"{$field}\""); + } + + preg_match_all('/\[([^\]]*)\]/', $matches[2], $segMatches); + $segments = array_merge([$matches[1]], $segMatches[1]); + + $leafField = array_pop($segments); + + $hops = []; + for ($i = 0; $i < count($segments); $i += 2) { + $hops[] = [$segments[$i], $segments[$i + 1] ?? null]; + } + + return [$hops, $leafField]; + } + + /** + * Walk hops like [['items','5'], ['taxes','2']] from $root, one + * relation call per hop. A non-numeric or missing id means an + * unsaved record still sitting in a deferred-binding session - a + * fresh instance stands in for it. + * + * Uses hasRelation() rather than method_exists(): Winter/October's + * conventional relation declaration style - `public $hasMany = + * ['items' => Item::class]` - resolves dynamically via __call() + * magic, not as a real PHP method, so method_exists() would + * incorrectly reject it. + */ + protected function resolveModelForHops($root, $hops) + { + $model = $root; + + foreach ($hops as [$relationName, $id]) { + $isValidRelation = method_exists($model, 'hasRelation') + ? $model->hasRelation($relationName) + : method_exists($model, $relationName); + + if (!$isValidRelation) { + throw new ApplicationException( + "Unknown relation \"{$relationName}\" while resolving nested field \"{$this->nestedField}\"" + ); + } + + $related = $model->{$relationName}()->getRelated(); + + $model = is_numeric($id) + ? ($related->newQuery()->find($id) ?: $related->newInstance()) + : $related->newInstance(); + } + + return $model; + } + + /** + * Config is keyed by the id-stripped bracket path (e.g. + * "items[taxes]"), but config lookup elsewhere in this class uses + * the bare leaf field name ("taxes"). Alias the bare name onto the + * qualified block in $this->originalConfig before it's consulted - + * the one stable place holding every field's definition at once + * ($this->config gets overwritten per-field on every + * initRelation() call). + */ + protected function aliasNestedConfig($leafField) + { + if (!$this->isNestedField()) { + return; + } + + $configKey = $this->getNestedConfigKey($this->nestedField); + + if ($configKey === $leafField) { + return; + } + + if (is_array($this->originalConfig)) { + if (!array_key_exists($configKey, $this->originalConfig)) { + throw new ApplicationException( + "No relation config entry found for \"{$configKey}\" ". + "(needed for nested field \"{$this->nestedField}\")." + ); + } + $this->originalConfig[$leafField] = $this->originalConfig[$configKey]; + return; + } + + if (is_object($this->originalConfig)) { + if (!isset($this->originalConfig->{$configKey})) { + throw new ApplicationException( + "No relation config entry found for \"{$configKey}\" ". + "(needed for nested field \"{$this->nestedField}\")." + ); + } + $this->originalConfig->{$leafField} = $this->originalConfig->{$configKey}; + return; + } + + throw new ApplicationException( + 'Unrecognised $this->originalConfig type: ' . gettype($this->originalConfig) + ); + } + + /** + * "items[5][taxes]" -> "items[taxes]" (drop every id segment). + * + * For a RECURSIVE relation (the same field name appearing more + * than once in the chain - e.g. items[1][items][9][items], an + * item's own sub-item's own sub-item), naively concatenating every + * occurrence produces a key ("items[items][items]") that grows + * without bound as recursion goes deeper, which could never match + * a finite config file - a recursive relation's schema is defined + * ONCE and reused at every depth, not once per level. + * + * The same collapse is needed even when the repeat ISN'T at the + * very end - e.g. items[1][items][2][taxes] (a sub-item's own + * taxes) produces segments ['items','items','taxes']: the + * repeated pair is at the START, followed by a genuinely different + * leaf. That still needs to resolve to "items[taxes]", since a + * sub-item's taxes relation is identical in shape to a root-level + * item's - reusing the SAME config entry regardless of how deep + * the item itself was reached, not a new one per depth. + * + * Finds and collapses ANY adjacent duplicate pair (not just a + * trailing one) down to a single occurrence, retrying until an + * existing config key is found or nothing more can be collapsed + * (falling back to the fully-expanded key so the caller's own + * "not found" error still reports something meaningful). + */ + protected function getNestedConfigKey($field) + { + [$hops, $leafField] = $this->parseBracketField($field); + + $segments = array_map(function ($hop) { + return $hop[0]; + }, $hops); + $segments[] = $leafField; + + while (true) { + $key = $segments[0]; + for ($i = 1; $i < count($segments); $i++) { + $key .= '[' . $segments[$i] . ']'; + } + + if ($this->nestedConfigKeyExists($key)) { + return $key; + } + + $collapsedAny = false; + + for ($i = 1; $i < count($segments); $i++) { + if ($segments[$i] === $segments[$i - 1]) { + array_splice($segments, $i, 1); + $collapsedAny = true; + break; + } + } + + if (!$collapsedAny) { + return $key; + } + } + } + + /** + * Existence check used by getNestedConfigKey()'s collapse loop - + * separate from aliasNestedConfig()'s own check, since that one + * needs to throw a clear error on genuine failure, while this one + * just needs a yes/no to decide whether to keep collapsing. + */ + protected function nestedConfigKeyExists($key) + { + if (is_array($this->originalConfig)) { + return array_key_exists($key, $this->originalConfig); + } + + if (is_object($this->originalConfig)) { + return isset($this->originalConfig->{$key}); + } + + return false; + } + + /** + * Resolves the id to use for a manage-form popup's own hop in the + * ambient stack - i.e. which specific record it pertains to, so + * nested fields under it can compute their own path. + * + * $this->manageId covers the common case (editing an existing + * record, or a plain create with no id yet). For a belongsToMany + * pivot link being created against an ALREADY-EXISTING foreign + * record (manageId absent, foreignId present - the popup is + * pre-populated with that record's data before the link itself is + * saved, per makePivotWidget()), $this->foreignId is what actually + * identifies the record; falling through to 'new' would + * incorrectly treat it as having no identity at all. foreignId can + * arrive as an array (makePivotWidget() itself casts it that way), + * hence reset(). + */ + protected function resolveHopId() + { + if ($this->manageId) { + return $this->manageId; + } + + if ($this->foreignId) { + return is_array($this->foreignId) ? reset($this->foreignId) : $this->foreignId; + } + + return 'new'; + } + + /** + * manage_id is never legitimate for a genuine create/add action (a + * "Create" should never have a pre-existing id, but stock's JS + * keeps one shared hidden input regardless of which container is + * involved), and never THIS field's own value when ambient (a side + * effect of rendering the PARENT's form, not what the request + * targets). + */ + protected function shouldSuppressManageId($isAmbient) + { + return $isAmbient || in_array($this->eventTarget, ['button-create', 'button-add']); + } + + /** + * foreign_id is only suppressed when ambient - unlike manage_id, + * it's legitimately needed for a genuine, direct pivot "Add" action + * (makePivotWidget() uses it to pre-populate the form with an + * already-existing foreign record); suppressing it there would + * break real pivot-add flows, not just stale leakage. + */ + protected function shouldSuppressForeignId($isAmbient) + { + return $isAmbient; + } + /** * Renders the relationship manager. * @param string $field The relationship field. @@ -516,12 +956,38 @@ public function relationRenderView($field = null) */ public function relationMakePartial($partial, $params = []) { - $contents = $this->controller->makePartial('relation_'.$partial, $params + $this->vars, false); - if (!$contents) { - $contents = $this->makePartial($partial, $params); + if ($partial !== 'manage_form' && $partial !== 'manage_pivot') { + $contents = $this->controller->makePartial('relation_'.$partial, $params + $this->vars, false); + if (!$contents) { + $contents = $this->makePartial($partial, $params); + } + + return $contents; } - return $contents; + // 'manage_form' (create/update popups) and 'manage_pivot' + // (belongsToMany with a `pivot` config block) both render a + // real Backend\Widgets\Form that could itself contain a + // further nested relation field - pushing/popping the ambient + // path here is what lets initRelation() recognise such a field + // by its position in this stack, rather than needing to + // reconstruct it from scratch. 'manage_list' is deliberately + // excluded: a plain link/unlink picker only ever renders list + // columns, never a full model form. + $hop = $this->nestedField . '[' . $this->resolveHopId() . ']'; + + array_push($this->nestingStack, $hop); + + try { + $contents = $this->controller->makePartial('relation_'.$partial, $params + $this->vars, false); + if (!$contents) { + $contents = $this->makePartial($partial, $params); + } + + return $contents; + } finally { + array_pop($this->nestingStack); + } } /** @@ -540,6 +1006,14 @@ public function relationGetId($suffix = null) $id .= '-' . $suffix; } + // Two instances of the same field name at different tree + // positions (e.g. "taxes" under two different items) would + // otherwise collide on DOM id - suffixed with a hash of the + // full bracket path, distinct per position. + if ($this->isNestedField()) { + $id .= '-' . substr(md5($this->nestedField), 0, 8); + } + return $this->controller->getId($id); } @@ -548,19 +1022,39 @@ public function relationGetId($suffix = null) */ public function relationGetSessionKey($force = false) { - if ($this->sessionKey && !$force) { - return $this->sessionKey; + if (!$this->isNestedField()) { + // Unmodified stock behaviour. + if ($this->sessionKey && !$force) { + return $this->sessionKey; + } + + if (post('_relation_session_key')) { + return $this->sessionKey = post('_relation_session_key'); + } + + if (post('_session_key')) { + return $this->sessionKey = post('_session_key'); + } + + return $this->sessionKey = FormHelper::getSessionKey(); } - if (post('_relation_session_key')) { - return $this->sessionKey = post('_relation_session_key'); + // $this->sessionKey is a single shared property - whichever + // field is resolved first in a request sets it, and the stock + // logic above would otherwise hand that SAME stale key back to + // every subsequently-resolved field in the request, rather + // than generating its own. Cached per $nestedField instead. + if (isset($this->sessionKeysByField[$this->nestedField]) && !$force) { + return $this->sessionKey = $this->sessionKeysByField[$this->nestedField]; } - if (post('_session_key')) { - return $this->sessionKey = post('_session_key'); + if ($posted = (post('_relation_session_key') ?: post('_session_key'))) { + return $this->sessionKey = $this->sessionKeysByField[$this->nestedField] = $posted; } - return $this->sessionKey = FormHelper::getSessionKey(); + $key = FormHelper::getSessionKey() . '.' . substr(md5($this->nestedField), 0, 12); + + return $this->sessionKey = $this->sessionKeysByField[$this->nestedField] = $key; } /** @@ -1853,7 +2347,17 @@ protected function evalManageTitle() */ protected function evalManageMode() { - if ($mode = post(self::PARAM_MODE)) { + // `_relation_mode` is a single global POST value with no + // per-field scoping. Trusting it unconditionally means a stale + // mode from an OUTER field (e.g. "form", correctly describing + // an open Item popup) can force itself onto a completely + // different action on a NESTED field (e.g. deleting a Tax), + // triggering a manage-widget lookup against an unrelated id. + // Ignored for nested fields - the switches below determine the + // right mode fresh from $this->eventTarget/$this->relationType + // regardless, so there's no legitimate case where a nested + // field needs the outer field's posted mode honoured. + if (!$this->isNestedField() && $mode = post(self::PARAM_MODE)) { return $mode; } diff --git a/modules/backend/behaviors/relationcontroller/partials/_button_delete.php b/modules/backend/behaviors/relationcontroller/partials/_button_delete.php index d8df34c81d..c297c42e1f 100644 --- a/modules/backend/behaviors/relationcontroller/partials/_button_delete.php +++ b/modules/backend/behaviors/relationcontroller/partials/_button_delete.php @@ -3,7 +3,7 @@ class="btn btn-sm btn-secondary wn-icon-trash-o" data-request="onRelationButtonDelete" data-request-confirm="" - data-request-success="$.wn.relationBehavior.changed('', 'deleted')" + data-request-success="$.wn.relationBehavior.changed('', 'deleted')" data-stripe-load-indicator> @@ -16,7 +16,7 @@ class="btn btn-sm btn-secondary wn-icon-trash-o" disabled="disabled" data-request="onRelationButtonDelete" data-request-confirm="" - data-request-success="$.wn.relationBehavior.changed('', 'deleted')" + data-request-success="$.wn.relationBehavior.changed('', 'deleted')" data-trigger-action="enable" data-trigger="#relationGetId('view') ?> .control-list input[type=checkbox]" data-trigger-condition="checked" diff --git a/modules/backend/behaviors/relationcontroller/partials/_button_remove.php b/modules/backend/behaviors/relationcontroller/partials/_button_remove.php index a5326e4f4d..683b917a54 100644 --- a/modules/backend/behaviors/relationcontroller/partials/_button_remove.php +++ b/modules/backend/behaviors/relationcontroller/partials/_button_remove.php @@ -2,7 +2,7 @@ @@ -14,7 +14,7 @@ class="btn btn-sm btn-secondary wn-icon-minus" })" disabled="disabled" data-request="onRelationButtonRemove" - data-request-success="$.wn.relationBehavior.changed('', 'removed')" + data-request-success="$.wn.relationBehavior.changed('', 'removed')" data-trigger-action="enable" data-trigger="#relationGetId('view') ?> .control-list input[type=checkbox]" data-trigger-condition="checked" diff --git a/modules/backend/behaviors/relationcontroller/partials/_button_unlink.php b/modules/backend/behaviors/relationcontroller/partials/_button_unlink.php index 59ea3ec221..acd54e55e7 100644 --- a/modules/backend/behaviors/relationcontroller/partials/_button_unlink.php +++ b/modules/backend/behaviors/relationcontroller/partials/_button_unlink.php @@ -2,7 +2,7 @@ href="javascript:;" class="btn btn-sm btn-secondary wn-icon-unlink" data-request="onRelationButtonUnlink" - data-request-success="$.wn.relationBehavior.changed('', 'removed')" + data-request-success="$.wn.relationBehavior.changed('', 'removed')" data-request-confirm="" data-stripe-load-indicator> diff --git a/modules/backend/formwidgets/RelationManager.php b/modules/backend/formwidgets/RelationManager.php index b1165885a4..1fcf1ea733 100644 --- a/modules/backend/formwidgets/RelationManager.php +++ b/modules/backend/formwidgets/RelationManager.php @@ -74,6 +74,20 @@ public function render() $relation = $this->relation ?: $this->formField->fieldName; + // Explicitly binds the controller's relation context to THIS + // widget's own model/field pair before rendering. Needed for a + // nested relation manager (one rendered inside another relation + // manager's own manage form): without this, relationRender()'s + // own internal validateField() check would lazily call + // initRelation() using $this->model - RelationController's own, + // currently-active model, which for a nested field is still + // whatever the OUTER field left it as, not this widget's own + // bound model. Harmless and idempotent for a root-level field + // (the widget's model and the controller's current model are + // already the same one), so this applies at any depth + // uniformly, with no special-casing needed here. + $this->controller->initRelation($this->model, $relation); + return $this->controller->relationRender($relation, $options); } diff --git a/modules/backend/tests/behaviors/RelationControllerNestedTest.php b/modules/backend/tests/behaviors/RelationControllerNestedTest.php new file mode 100644 index 0000000000..a297b37702 --- /dev/null +++ b/modules/backend/tests/behaviors/RelationControllerNestedTest.php @@ -0,0 +1,842 @@ + Item -(taxes, belongsToMany)-> Tax + * | + * `-(items, hasMany, self-referencing)-> Item + * + * The self-referencing relation on Item is what exercises recursion: + * the same field name ("items") appears at two different depths. + */ +class RelationTestOrder extends Model +{ + public $table = 'backend_test_relation_orders'; + protected $guarded = []; + public $timestamps = false; + + public $hasMany = [ + 'items' => [RelationTestItem::class, 'key' => 'order_id'], + ]; + + public static function migrateUp(): void + { + if (Schema::hasTable('backend_test_relation_orders')) { + return; + } + + Schema::create('backend_test_relation_orders', function ($table) { + $table->increments('id'); + $table->string('name')->nullable(); + }); + } + + public static function migrateDown(): void + { + Schema::dropIfExists('backend_test_relation_orders'); + } +} + +class RelationTestItem extends Model +{ + public $table = 'backend_test_relation_items'; + protected $guarded = []; + public $timestamps = false; + + public $belongsTo = [ + 'order' => [RelationTestOrder::class, 'key' => 'order_id'], + 'parentItem' => [RelationTestItem::class, 'key' => 'parent_item_id'], + ]; + + public $hasMany = [ + // Self-referencing: an Item's own sub-items. Deliberately the + // SAME field name ("items") as Order's own relation, so tests + // can exercise a recursive relation - the same name reused at + // two different depths - not just a same-named coincidence. + 'items' => [RelationTestItem::class, 'key' => 'parent_item_id'], + ]; + + public $belongsToMany = [ + 'taxes' => [ + RelationTestTax::class, + 'table' => 'backend_test_relation_item_tax', + 'key' => 'item_id', + 'otherKey' => 'tax_id', + ], + ]; + + public static function migrateUp(): void + { + if (Schema::hasTable('backend_test_relation_items')) { + return; + } + + Schema::create('backend_test_relation_items', function ($table) { + $table->increments('id'); + $table->unsignedInteger('order_id')->nullable(); + $table->unsignedInteger('parent_item_id')->nullable(); + $table->string('name')->nullable(); + $table->integer('quantity')->nullable(); + }); + + Schema::create('backend_test_relation_item_tax', function ($table) { + $table->unsignedInteger('item_id'); + $table->unsignedInteger('tax_id'); + }); + } + + public static function migrateDown(): void + { + Schema::dropIfExists('backend_test_relation_item_tax'); + Schema::dropIfExists('backend_test_relation_items'); + } +} + +class RelationTestTax extends Model +{ + public $table = 'backend_test_relation_taxes'; + protected $guarded = []; + public $timestamps = false; + + public $belongsToMany = [ + 'items' => [ + RelationTestItem::class, + 'table' => 'backend_test_relation_item_tax', + 'key' => 'tax_id', + 'otherKey' => 'item_id', + ], + ]; + + public static function migrateUp(): void + { + if (Schema::hasTable('backend_test_relation_taxes')) { + return; + } + + Schema::create('backend_test_relation_taxes', function ($table) { + $table->increments('id'); + $table->string('label')->nullable(); + $table->integer('rate')->nullable(); + }); + } + + public static function migrateDown(): void + { + Schema::dropIfExists('backend_test_relation_taxes'); + } +} + +/** + * Minimal backend controller implementing RelationController directly + * (not a subclass, since nested support is now native). No FormController + * implementation is needed - resolveFallbackRootModel() checks for a + * formGetModel() method directly, which this stubs. + * + * config_relation.yaml keys use the id-stripped bracket path + * ("items[taxes]", "items[items]"), matching real usage - see + * RelationController::getNestedConfigKey(). + */ +class RelationTestController extends BackendController +{ + public $implement = [ + RelationController::class, + ]; + + public $relationConfig = [ + 'items' => [ + 'label' => 'Items', + 'view' => [ + 'list' => ['columns' => ['name' => ['label' => 'Name']]], + ], + 'manage' => [ + 'form' => ['fields' => [ + 'name' => ['label' => 'Name', 'type' => 'text'], + 'quantity' => ['label' => 'Quantity', 'type' => 'number'], + // Rendered AMBIENTLY (same-request) whenever an + // Item's own manage form renders - this is what + // exercises resolveNestedContext()'s case (a) and + // relationMakePartial()'s stack push/pop, as + // opposed to every other test in this file, which + // posts an already-qualified bracket path and only + // exercises case (b) reconstruction. + 'taxes' => ['label' => 'Taxes', 'type' => 'relationmanager'], + ]], + ], + ], + 'items[taxes]' => [ + 'label' => 'Taxes', + 'view' => [ + 'list' => ['columns' => ['label' => ['label' => 'Label']]], + ], + 'manage' => [ + 'form' => ['fields' => [ + 'label' => ['label' => 'Label', 'type' => 'text'], + 'rate' => ['label' => 'Rate', 'type' => 'number'], + ]], + ], + ], + 'items[items]' => [ + 'label' => 'Sub-items', + 'view' => [ + 'list' => ['columns' => ['name' => ['label' => 'Name']]], + ], + 'manage' => [ + 'form' => ['fields' => [ + 'name' => ['label' => 'Name', 'type' => 'text'], + 'quantity' => ['label' => 'Quantity', 'type' => 'number'], + ]], + ], + ], + ]; + + protected $testRootModel; + + public function setTestRootModel($model) + { + $this->testRootModel = $model; + } + + /** + * Stubs the conventional FormController accessor that + * resolveFallbackRootModel() looks for - stands in for a full + * FormController implementation, which isn't needed just to + * exercise RelationController itself. + */ + public function formGetModel() + { + return $this->testRootModel; + } +} + +class RelationControllerNestedTest extends PluginTestCase +{ + public function setUp(): void + { + parent::setUp(); + + RelationTestOrder::migrateUp(); + RelationTestItem::migrateUp(); + RelationTestTax::migrateUp(); + + $this->actingAs((new UserFixture)->asSuperUser()); + } + + public function tearDown(): void + { + RelationTestTax::migrateDown(); + RelationTestItem::migrateDown(); + RelationTestOrder::migrateDown(); + + parent::tearDown(); + } + + /** + * Simulates a POST request the way ListsSortableTest does - direct + * handler invocation, not a full $controller->run() dispatch. + * pageAction() (called by beforeAjax()) safely no-ops when + * $this->action is unset, which it always is here - matching the + * real-world case a nested field hits on every fresh AJAX request + * (see resolveFallbackRootModel()'s own docblock). + */ + protected function postRequest(array $data): void + { + $request = HttpRequest::create('/', 'POST', $data); + $this->app->instance('request', $request); + \Request::swap($request); + } + + protected function makeController(Model $rootModel): RelationTestController + { + $controller = new RelationTestController; + $controller->setTestRootModel($rootModel); + + return $controller; + } + + // ----------------------------------------------------------------- + // Regression: root-level (non-nested) behaviour must be unaffected. + // This is the highest-stakes check for a change to a shared core + // class - the overwhelming majority of real installs only ever + // exercise this path. + // ----------------------------------------------------------------- + + public function testRootLevelCreateAttachesToRootModel() + { + $order = RelationTestOrder::create(['name' => 'Order 1']); + $controller = $this->makeController($order); + + $this->postRequest([ + '_relation_field' => 'items', + 'RelationTestItem' => ['name' => 'Widget', 'quantity' => 5], + ]); + + $controller->onRelationManageCreate(); + + $order = RelationTestOrder::find($order->id); + $this->assertCount(1, $order->items); + $this->assertEquals('Widget', $order->items->first()->name); + $this->assertEquals(5, $order->items->first()->quantity); + } + + public function testRootLevelUpdateModifiesCorrectRecord() + { + $order = RelationTestOrder::create(['name' => 'Order 1']); + $item = RelationTestItem::create(['order_id' => $order->id, 'name' => 'Original', 'quantity' => 1]); + + $controller = $this->makeController($order); + + $this->postRequest([ + '_relation_field' => 'items', + 'manage_id' => $item->id, + 'RelationTestItem' => ['name' => 'Updated', 'quantity' => 9], + ]); + + $controller->onRelationManageUpdate(); + + $item = RelationTestItem::find($item->id); + $this->assertEquals('Updated', $item->name); + $this->assertEquals(9, $item->quantity); + } + + // ----------------------------------------------------------------- + // Nested: a relation manager rendered inside another's manage form. + // ----------------------------------------------------------------- + + public function testNestedCreateAttachesToCorrectItemNotOrder() + { + $order = RelationTestOrder::create(['name' => 'Order 1']); + $itemA = RelationTestItem::create(['order_id' => $order->id, 'name' => 'Item A']); + $itemB = RelationTestItem::create(['order_id' => $order->id, 'name' => 'Item B']); + + $controller = $this->makeController($order); + + $this->postRequest([ + '_relation_field' => "items[{$itemA->id}][taxes]", + 'RelationTestTax' => ['label' => 'GST', 'rate' => 5], + ]); + + $controller->onRelationManageCreate(); + + $itemA = RelationTestItem::find($itemA->id); + $itemB = RelationTestItem::find($itemB->id); + + $this->assertCount(1, $itemA->taxes, 'Tax should attach to the item named in the qualified field path'); + $this->assertEquals('GST', $itemA->taxes->first()->label); + $this->assertCount(0, $itemB->taxes, 'A sibling item must not receive the tax meant for a different item'); + } + + public function testNestedUpdateModifiesCorrectTaxRecord() + { + $order = RelationTestOrder::create(['name' => 'Order 1']); + $item = RelationTestItem::create(['order_id' => $order->id, 'name' => 'Item A']); + $tax = RelationTestTax::create(['label' => 'Original', 'rate' => 1]); + $item->taxes()->add($tax); + + $controller = $this->makeController($order); + + $this->postRequest([ + '_relation_field' => "items[{$item->id}][taxes]", + 'manage_id' => $tax->id, + 'RelationTestTax' => ['label' => 'Updated', 'rate' => 15], + ]); + + $controller->onRelationManageUpdate(); + + $tax = RelationTestTax::find($tax->id); + $this->assertEquals('Updated', $tax->label); + $this->assertEquals(15, $tax->rate); + } + + /** + * Regression for a real, once-broken bug: manage_id is a single + * global POST value with no per-field scoping. Without + * shouldSuppressManageId()'s create/add-action suppression, a + * stale manage_id left over from editing a DIFFERENT record + * elsewhere on the page could make a genuine "Create" action + * silently behave like an edit of that stale id instead. + * + * Must go through onRelationButtonCreate() specifically, not + * onRelationManageCreate() (the actual save handler) - the save + * logic creates the new record via $this->relationModel directly + * and never consults $this->manageId at all, so a test calling the + * save handler directly would pass even without the suppression + * fix in place. The real risk is earlier: makeManageWidget(), + * called unconditionally by initRelation() on EVERY handler call + * including this one, uses manage_id to decide whether to build a + * blank form or find() and pre-populate an existing record - a + * stale id there means the "Create" popup opens pre-filled with a + * record that has nothing to do with the field being created. + */ + public function testNestedCreatePopupDoesNotPrePopulateFromStaleManageId() + { + $order = RelationTestOrder::create(['name' => 'Order 1']); + $item = RelationTestItem::create(['order_id' => $order->id, 'name' => 'Item A']); + $unrelatedTax = RelationTestTax::create(['label' => 'Should not be pre-populated', 'rate' => 99]); + + $controller = $this->makeController($order); + + $this->postRequest([ + '_relation_field' => "items[{$item->id}][taxes]", + // Stale manage_id, as if left over from editing a + // different record elsewhere on the page - a genuine + // "Create" click should never honour this. + 'manage_id' => $unrelatedTax->id, + ]); + + $html = $controller->onRelationButtonCreate(); + + $this->assertIsString($html); + $this->assertStringNotContainsString( + 'Should not be pre-populated', + $html, + 'A stale manage_id must not pre-populate a "Create" popup with an unrelated record\'s data' + ); + } + + // ----------------------------------------------------------------- + // Recursive nesting: the same field name ("items") reused at two + // depths. This is what exercises getNestedConfigKey()'s collapse + // logic - without it, resolving config for a sub-item's own + // sub-item would look for a config key that can never exist. + // ----------------------------------------------------------------- + + public function testRecursiveNestingAttachesSubItemToCorrectParentItem() + { + $order = RelationTestOrder::create(['name' => 'Order 1']); + $parentItem = RelationTestItem::create(['order_id' => $order->id, 'name' => 'Parent']); + $siblingItem = RelationTestItem::create(['order_id' => $order->id, 'name' => 'Sibling']); + + $controller = $this->makeController($order); + + $this->postRequest([ + '_relation_field' => "items[{$parentItem->id}][items]", + 'RelationTestItem' => ['name' => 'Sub-item', 'quantity' => 3], + ]); + + $controller->onRelationManageCreate(); + + $parentItem = RelationTestItem::find($parentItem->id); + $siblingItem = RelationTestItem::find($siblingItem->id); + + $this->assertCount(1, $parentItem->items, 'Sub-item should attach to its intended parent item'); + $this->assertEquals('Sub-item', $parentItem->items->first()->name); + $this->assertCount(0, $siblingItem->items, 'A sibling item must not receive a sub-item meant for another item'); + } + + public function testThreeLevelNestingResolvesCorrectAncestor() + { + $order = RelationTestOrder::create(['name' => 'Order 1']); + $item = RelationTestItem::create(['order_id' => $order->id, 'name' => 'Item']); + $subItem = RelationTestItem::create(['parent_item_id' => $item->id, 'name' => 'Sub-item']); + $unrelatedSubItem = RelationTestItem::create(['parent_item_id' => $item->id, 'name' => 'Other sub-item']); + + $controller = $this->makeController($order); + + // Three hops deep: order -> item -> sub-item -> tax. + $this->postRequest([ + '_relation_field' => "items[{$item->id}][items][{$subItem->id}][taxes]", + 'RelationTestTax' => ['label' => 'Deep Tax', 'rate' => 12], + ]); + + $controller->onRelationManageCreate(); + + $subItem = RelationTestItem::find($subItem->id); + $unrelatedSubItem = RelationTestItem::find($unrelatedSubItem->id); + + $this->assertCount(1, $subItem->taxes, 'Tax should attach three hops down to the correct sub-item'); + $this->assertEquals('Deep Tax', $subItem->taxes->first()->label); + $this->assertCount(0, $unrelatedSubItem->taxes, 'A sibling sub-item at the same depth must not receive the tax'); + } + + // ----------------------------------------------------------------- + // Unit-level tests for the pure parsing/resolution algorithms, + // via reflection since they're protected methods with no side + // effects worth the overhead of a full request simulation. + // ----------------------------------------------------------------- + + protected function invokeProtectedMethod($object, string $method, array $args = []) + { + $reflection = new \ReflectionMethod(get_class($object), $method); + $reflection->setAccessible(true); + + return $reflection->invokeArgs($object, $args); + } + + protected function readProtectedProperty($object, string $property) + { + $reflection = new \ReflectionProperty(get_class($object), $property); + $reflection->setAccessible(true); + + return $reflection->getValue($object); + } + + public function testParseBracketFieldSplitsHopsAndLeaf() + { + $order = RelationTestOrder::create(['name' => 'Order 1']); + $controller = $this->makeController($order); + + [$hops, $leaf] = $this->invokeProtectedMethod($controller->asExtension(RelationController::class), 'parseBracketField', [ + 'items[5][taxes][2][adjustments]', + ]); + + $this->assertEquals([['items', '5'], ['taxes', '2']], $hops); + $this->assertEquals('adjustments', $leaf); + } + + public function testParseBracketFieldRejectsMalformedInput() + { + $order = RelationTestOrder::create(['name' => 'Order 1']); + $controller = $this->makeController($order); + $behavior = $controller->asExtension(RelationController::class); + + $this->expectException(\Winter\Storm\Exception\ApplicationException::class); + + $this->invokeProtectedMethod($behavior, 'parseBracketField', ['not a valid bracket field']); + } + + /** + * The core fix for recursive relations: naively concatenating every + * hop produces a config key that grows without bound as recursion + * goes deeper ("items[items][items]", "items[items][items][items]", + * ...), which could never match a finite config file. A trailing + * repeated segment must collapse to one occurrence regardless of + * how many times it repeats. + */ + public function testGetNestedConfigKeyCollapsesRecursiveRepeatsAtAnyDepth() + { + $order = RelationTestOrder::create(['name' => 'Order 1']); + $controller = $this->makeController($order); + $behavior = $controller->asExtension(RelationController::class); + + // Force originalConfig into a known state matching our test + // controller's own $relationConfig, since getNestedConfigKey() + // checks against it directly. + $reflection = new \ReflectionProperty($behavior, 'originalConfig'); + $reflection->setAccessible(true); + $originalConfig = $reflection->getValue($behavior); + + $this->assertTrue( + isset($originalConfig->{'items[items]'}), + 'Test controller config must define items[items] for this test to be meaningful' + ); + + $twoLevels = $this->invokeProtectedMethod($behavior, 'getNestedConfigKey', ['items[1][items][2][items]']); + $threeLevels = $this->invokeProtectedMethod($behavior, 'getNestedConfigKey', ['items[1][items][2][items][3][items]']); + $fourLevels = $this->invokeProtectedMethod($behavior, 'getNestedConfigKey', ['items[1][items][2][items][3][items][4][items]']); + + $this->assertEquals('items[items]', $twoLevels); + $this->assertEquals('items[items]', $threeLevels, 'Collapse must apply regardless of how deep the recursion goes'); + $this->assertEquals('items[items]', $fourLevels, 'Collapse must apply regardless of how deep the recursion goes'); + } + + public function testGetNestedConfigKeyLeavesNonRecursivePathsUnaffected() + { + $order = RelationTestOrder::create(['name' => 'Order 1']); + $controller = $this->makeController($order); + $behavior = $controller->asExtension(RelationController::class); + + $key = $this->invokeProtectedMethod($behavior, 'getNestedConfigKey', ['items[1][taxes]']); + + $this->assertEquals('items[taxes]', $key); + } + + // ----------------------------------------------------------------- + // Regression: validateField() must prefer an already-active field + // over stale POST data, since relationRenderToolbar()/ + // relationRenderView()/relationRefresh() all call it with no field + // argument at all, historically assuming the POST value + // unambiguously identifies "the" active field - false the moment a + // nested field is active alongside its ancestor within one request. + // ----------------------------------------------------------------- + + public function testRelationRefreshDoesNotLoseNestedFieldContext() + { + $order = RelationTestOrder::create(['name' => 'Order 1']); + $item = RelationTestItem::create(['order_id' => $order->id, 'name' => 'Item A']); + + $controller = $this->makeController($order); + + // _relation_field carries the full qualified path, as a real + // nested request would - relationRefresh()'s own internal + // validateField(null) call must not regress $this->field back + // to the raw bracket path once it's already been narrowed to + // the bare leaf by the create itself. + $this->postRequest([ + '_relation_field' => "items[{$item->id}][taxes]", + 'RelationTestTax' => ['label' => 'GST', 'rate' => 5], + ]); + + // onRelationManageCreate() ends with $this->relationRefresh() - + // if this throws or resolves the wrong field, that's the bug + // this test guards against. + $result = $controller->onRelationManageCreate(); + + $this->assertIsArray($result); + + $item = RelationTestItem::find($item->id); + $this->assertCount(1, $item->taxes); + } + + // ----------------------------------------------------------------- + // Popup-opening handlers: onRelationButtonCreate()/ + // onRelationButtonUpdate(), which set $this->eventTarget and + // delegate to onRelationManageForm() -> makeManageWidget(). Every + // other test in this file exercises SAVE handlers + // (onRelationManageCreate()/onRelationManageUpdate()), which never + // consult manage_id or eventTarget the same way - these are the + // actual site of the manage_id/eventTarget-driven bugs, and were + // entirely uncovered before this addition. + // ----------------------------------------------------------------- + + public function testNestedButtonCreateBuildsBlankFormForCorrectRelation() + { + $order = RelationTestOrder::create(['name' => 'Order 1']); + $item = RelationTestItem::create(['order_id' => $order->id, 'name' => 'Item A']); + + $controller = $this->makeController($order); + + $this->postRequest([ + '_relation_field' => "items[{$item->id}][taxes]", + ]); + + $html = $controller->onRelationButtonCreate(); + $behavior = $controller->asExtension(RelationController::class); + $manageMode = $this->readProtectedProperty($behavior, 'manageMode'); + $manageWidget = $this->readProtectedProperty($behavior, 'manageWidget'); + + $this->assertIsString($html); + $this->assertEquals('form', $manageMode); + $this->assertInstanceOf(RelationTestTax::class, $manageWidget->model); + $this->assertFalse($manageWidget->model->exists, 'Create should always bind a blank, unsaved model'); + } + + public function testNestedButtonUpdateBindsCorrectExistingRecord() + { + $order = RelationTestOrder::create(['name' => 'Order 1']); + $item = RelationTestItem::create(['order_id' => $order->id, 'name' => 'Item A']); + $otherItem = RelationTestItem::create(['order_id' => $order->id, 'name' => 'Item B']); + $tax = RelationTestTax::create(['label' => 'GST', 'rate' => 5]); + $decoyTax = RelationTestTax::create(['label' => 'Should not be bound', 'rate' => 1]); + $item->taxes()->add($tax); + $otherItem->taxes()->add($decoyTax); + + $controller = $this->makeController($order); + + $this->postRequest([ + '_relation_field' => "items[{$item->id}][taxes]", + 'manage_id' => $tax->id, + ]); + + $html = $controller->onRelationButtonUpdate(); + $behavior = $controller->asExtension(RelationController::class); + $manageWidget = $this->readProtectedProperty($behavior, 'manageWidget'); + + $this->assertIsString($html); + $this->assertTrue($manageWidget->model->exists); + $this->assertEquals($tax->id, $manageWidget->model->id); + $this->assertEquals('GST', $manageWidget->model->label); + $this->assertStringNotContainsString('Should not be bound', $html); + } + + // ----------------------------------------------------------------- + // Ambient (same-request) nesting: a relation field rendered INLINE + // within another relation's own manage form, as opposed to every + // other test in this file, which posts an already-qualified + // bracket path and only ever exercises reconstruction from a fresh + // request. This is the other half of resolveNestedContext() (case + // "a") - entirely uncovered before this addition. + // + // Deliberately bypasses the full Backend\Widgets\Form field-type + // rendering pipeline (i.e. never actually renders a + // type: relationmanager field): that pipeline depends on widget + // registration having correctly booted, which is unrelated to + // anything RelationController itself is responsible for and + // doesn't reliably happen in a minimal PHPUnit bootstrap (confirmed + // - it fails with a widget-registration-shaped error even though + // this exact field type has been manually tested working + // correctly in a real browser many times over). Instead, this + // simulates exactly what a widget's render() call does - push a + // hop onto the stack the same way relationMakePartial() does, then + // call initRelation() with a bare field name while it's non-empty + // - which is the actual mechanism under test, independent of + // whether any particular field widget can render in this + // environment. + // ----------------------------------------------------------------- + + protected function pushNestingStack($behavior, string $hop): void + { + $reflection = new \ReflectionProperty(get_class($behavior), 'nestingStack'); + $reflection->setAccessible(true); + $stack = $reflection->getValue($behavior); + $stack[] = $hop; + $reflection->setValue($behavior, $stack); + } + + protected function popNestingStack($behavior): void + { + $reflection = new \ReflectionProperty(get_class($behavior), 'nestingStack'); + $reflection->setAccessible(true); + $stack = $reflection->getValue($behavior); + array_pop($stack); + $reflection->setValue($behavior, $stack); + } + + public function testAmbientFieldQualifiesUsingStackTopAsParentContext() + { + $order = RelationTestOrder::create(['name' => 'Order 1']); + $item = RelationTestItem::create(['order_id' => $order->id, 'name' => 'Item A']); + + $controller = $this->makeController($order); + $behavior = $controller->asExtension(RelationController::class); + + // Establishes "items" as the currently-active field, the same + // way validateField()/initRelation() would from a real request + // - needed so aliasNestedConfig() and friends have a coherent + // $this->field to work from during the nested call below. + $controller->initRelation($order, 'items'); + + // Simulates relationMakePartial() actively rendering Item's + // own manage form - the exact condition resolveNestedContext() + // checks for via end($this->nestingStack). + $this->pushNestingStack($behavior, "items[{$item->id}]"); + + // What RelationManager::render() does: initRelation() called + // with the widget's own bound model and a BARE field name, no + // bracket path involved at all. + $controller->initRelation($item, 'taxes'); + + $this->popNestingStack($behavior); + + $nestedField = $this->readProtectedProperty($behavior, 'nestedField'); + + $this->assertEquals( + "items[{$item->id}][taxes]", + $nestedField, + 'A field resolved while the stack is non-empty should be qualified using the stack\'s top entry as its parent context' + ); + } + + public function testAmbientFieldDistinguishesBetweenSiblingItems() + { + $order = RelationTestOrder::create(['name' => 'Order 1']); + $itemA = RelationTestItem::create(['order_id' => $order->id, 'name' => 'Item A']); + $itemB = RelationTestItem::create(['order_id' => $order->id, 'name' => 'Item B']); + + $controller = $this->makeController($order); + $behavior = $controller->asExtension(RelationController::class); + $controller->initRelation($order, 'items'); + + $this->pushNestingStack($behavior, "items[{$itemA->id}]"); + $controller->initRelation($itemA, 'taxes'); + $nestedFieldForA = $this->readProtectedProperty($behavior, 'nestedField'); + $this->popNestingStack($behavior); + + $this->pushNestingStack($behavior, "items[{$itemB->id}]"); + $controller->initRelation($itemB, 'taxes'); + $nestedFieldForB = $this->readProtectedProperty($behavior, 'nestedField'); + $this->popNestingStack($behavior); + + $this->assertEquals("items[{$itemA->id}][taxes]", $nestedFieldForA); + $this->assertEquals("items[{$itemB->id}][taxes]", $nestedFieldForB); + $this->assertNotEquals($nestedFieldForA, $nestedFieldForB, 'Two sibling items\' ambient contexts must resolve to distinct qualified fields'); + } + + /** + * Narrower test for relationMakePartial() specifically - confirms + * it actually pushes before rendering and pops after, regardless + * of whether the render call inside it succeeds (it has its own + * try/finally around the pop). The render itself may well fail in + * this environment (same widget-registration issue as the tests + * above), which is fine - this test only cares about stack state + * after the call returns, not whether rendering succeeded. + */ + public function testRelationMakePartialPushesAndPopsStackAroundManageFormRender() + { + $order = RelationTestOrder::create(['name' => 'Order 1']); + $item = RelationTestItem::create(['order_id' => $order->id, 'name' => 'Item A']); + + $controller = $this->makeController($order); + $behavior = $controller->asExtension(RelationController::class); + $controller->initRelation($order, 'items'); + + // A partial name relationMakePartial() doesn't special-case + // (only 'manage_form'/'manage_pivot' trigger push/pop) - used + // here purely as a vehicle to inspect stack state mid-call via + // a bound closure, without needing the real partial to exist. + $reflection = new \ReflectionProperty(get_class($behavior), 'nestingStack'); + $reflection->setAccessible(true); + + try { + $controller->relationMakePartial('manage_form'); + } catch (\Throwable $e) { + // Rendering itself may fail in this environment (same + // widget-registration issue as the tests above) - that's + // not what this test is checking. + } + + $stackAfterRender = $reflection->getValue($behavior); + + $this->assertEmpty($stackAfterRender, 'Stack must be empty again after relationMakePartial() returns, even if the render itself failed or threw'); + } + + // ----------------------------------------------------------------- + // Regression: _relation_mode is a single global POST value with no + // per-field scoping. evalManageMode()'s first check trusts it + // unconditionally, so a stale mode from an OUTER field (correctly + // "form", since editing an Item IS a form) can force itself onto a + // nested field's own, unrelated action - the real bug manifested as + // makeManageWidget() attempting to find() a Tax using the outer + // Item's own id, thrown from deep inside beforeAjax() before the + // handler's own logic ever ran. + // ----------------------------------------------------------------- + + public function testStaleRelationModeDoesNotBreakNestedDelete() + { + $order = RelationTestOrder::create(['name' => 'Order 1']); + // Decoy records first, so the item and tax ids are guaranteed + // to diverge - otherwise both tables' autoincrement could + // coincidentally start at the same value and mask the bug. + RelationTestItem::create(['order_id' => $order->id, 'name' => 'Decoy 1']); + RelationTestItem::create(['order_id' => $order->id, 'name' => 'Decoy 2']); + $item = RelationTestItem::create(['order_id' => $order->id, 'name' => 'Item A']); + $tax = RelationTestTax::create(['label' => 'GST', 'rate' => 5]); + $item->taxes()->add($tax); + + $controller = $this->makeController($order); + + $this->postRequest([ + '_relation_field' => "items[{$item->id}][taxes]", + // Both stale, as if leaked from the OUTER 'items' field's + // own context: that field IS legitimately in 'form' mode + // (being edited), with manage_id correctly identifying the + // item itself - neither has any legitimate meaning for + // this nested delete action. + '_relation_mode' => 'form', + 'manage_id' => $item->id, + 'checked' => [$tax->id], + ]); + + // Without suppression, this throws from inside beforeAjax() + // itself (evalManageMode() incorrectly returns 'form' from the + // stale POST value, which makes makeManageWidget() attempt + // find($item->id) WITHIN THE TAXES TABLE) - before + // onRelationManageDelete()'s own delete logic ever runs. + $controller->onRelationButtonDelete(); + + $this->assertNull(RelationTestTax::find($tax->id), 'Tax should have been deleted despite the stale _relation_mode/manage_id'); + + $item = RelationTestItem::find($item->id); + $this->assertCount(0, $item->taxes); + } +} diff --git a/modules/system/assets/ui/js/popup.js b/modules/system/assets/ui/js/popup.js index c6b6b0ac0a..c0ec699dde 100644 --- a/modules/system/assets/ui/js/popup.js +++ b/modules/system/assets/ui/js/popup.js @@ -52,6 +52,11 @@ zIndex: null } + // Pixels shifted down and right per already-open popup when a new + // one is shown, so stacked popups cascade visually instead of + // landing exactly on top of one another. See show(). + Popup.STACK_OFFSET = 40 + Popup.prototype.init = function(){ var self = this @@ -304,6 +309,15 @@ } Popup.prototype.show = function() { + // Cascade stacked popups with a visible offset, so it's clear + // at a glance that a popup is nested on top of another rather + // than a fresh, unrelated dialog. Depth is simply how many + // OTHER popups are already open when this one shows - counted + // before triggering Bootstrap's own show, so this one is never + // counted against itself regardless of how quickly Bootstrap + // applies its own .in class. + var stackDepth = $('.control-popup.in').length + this.$modal.modal('show') this.$modal.on('click.dismiss.popup', '[data-dismiss="popup"]', $.proxy(this.hide, this)) @@ -313,6 +327,13 @@ // Fixes an issue where the Modal makes `position: fixed` elements relative to itself // https://github.com/twbs/bootstrap/issues/15856 this.$dialog.css('transform', 'inherit') + + if (stackDepth > 0) { + this.$dialog.css({ + top: (stackDepth * Popup.STACK_OFFSET) + 'px', + left: (stackDepth * Popup.STACK_OFFSET) + 'px' + }) + } } Popup.prototype.hide = function() { From 2c6357223f6d18ba59b5c5e0a3a72276b3e705f9 Mon Sep 17 00:00:00 2001 From: Marc Jauvin Date: Mon, 6 Jul 2026 09:42:01 -0400 Subject: [PATCH 2/8] regenerate storm-min.j --- modules/system/assets/ui/storm-min.js | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/modules/system/assets/ui/storm-min.js b/modules/system/assets/ui/storm-min.js index 12b40962a4..01ecdde1f7 100644 --- a/modules/system/assets/ui/storm-min.js +++ b/modules/system/assets/ui/storm-min.js @@ -2054,6 +2054,7 @@ this.init()} Popup.prototype=Object.create(BaseProto) Popup.prototype.constructor=Popup Popup.DEFAULTS={ajax:null,handler:null,keyboard:true,extraData:{},content:null,size:null,adaptiveHeight:false,zIndex:null} +Popup.STACK_OFFSET=40 Popup.prototype.init=function(){var self=this if(self.isOpen)return this.setBackdrop(true) @@ -2127,11 +2128,13 @@ Popup.prototype.triggerEvent=function(eventName,params){if(!params){params=[this this.$el.trigger(eventObject,params) if(this.firstDiv){this.firstDiv.trigger(eventObject,params)}} Popup.prototype.reload=function(){this.init()} -Popup.prototype.show=function(){this.$modal.modal('show') +Popup.prototype.show=function(){var stackDepth=$('.control-popup.in').length +this.$modal.modal('show') this.$modal.on('click.dismiss.popup','[data-dismiss="popup"]',$.proxy(this.hide,this)) this.triggerEvent('popupShow') this.triggerEvent('show.oc.popup') -this.$dialog.css('transform','inherit')} +this.$dialog.css('transform','inherit') +if(stackDepth>0){this.$dialog.css({top:(stackDepth*Popup.STACK_OFFSET)+'px',left:(stackDepth*Popup.STACK_OFFSET)+'px'})}} Popup.prototype.hide=function(){if(!this.isOpen)return this.triggerEvent('popupHide') this.triggerEvent('hide.oc.popup') From 8a1b4c236717b97edcb932b897e0bf50c0449b27 Mon Sep 17 00:00:00 2001 From: Marc Jauvin Date: Mon, 6 Jul 2026 09:49:37 -0400 Subject: [PATCH 3/8] Stale offset persists after popup is reused with a lower stack depth Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> --- modules/system/assets/ui/js/popup.js | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/modules/system/assets/ui/js/popup.js b/modules/system/assets/ui/js/popup.js index c0ec699dde..c978d05f75 100644 --- a/modules/system/assets/ui/js/popup.js +++ b/modules/system/assets/ui/js/popup.js @@ -328,12 +328,10 @@ // https://github.com/twbs/bootstrap/issues/15856 this.$dialog.css('transform', 'inherit') - if (stackDepth > 0) { - this.$dialog.css({ - top: (stackDepth * Popup.STACK_OFFSET) + 'px', - left: (stackDepth * Popup.STACK_OFFSET) + 'px' - }) - } + this.$dialog.css({ + top: stackDepth > 0 ? (stackDepth * Popup.STACK_OFFSET) + 'px' : '', + left: stackDepth > 0 ? (stackDepth * Popup.STACK_OFFSET) + 'px' : '' + }) } Popup.prototype.hide = function() { From 23ccc71fd72699d072ca11695aa88159527d5aa8 Mon Sep 17 00:00:00 2001 From: Marc Jauvin Date: Mon, 6 Jul 2026 09:53:16 -0400 Subject: [PATCH 4/8] regenerate js assets --- modules/system/assets/ui/storm-min.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/system/assets/ui/storm-min.js b/modules/system/assets/ui/storm-min.js index 01ecdde1f7..7467b90c73 100644 --- a/modules/system/assets/ui/storm-min.js +++ b/modules/system/assets/ui/storm-min.js @@ -2134,7 +2134,7 @@ this.$modal.on('click.dismiss.popup','[data-dismiss="popup"]',$.proxy(this.hide, this.triggerEvent('popupShow') this.triggerEvent('show.oc.popup') this.$dialog.css('transform','inherit') -if(stackDepth>0){this.$dialog.css({top:(stackDepth*Popup.STACK_OFFSET)+'px',left:(stackDepth*Popup.STACK_OFFSET)+'px'})}} +this.$dialog.css({top:stackDepth>0?(stackDepth*Popup.STACK_OFFSET)+'px':'',left:stackDepth>0?(stackDepth*Popup.STACK_OFFSET)+'px':''})} Popup.prototype.hide=function(){if(!this.isOpen)return this.triggerEvent('popupHide') this.triggerEvent('hide.oc.popup') From 5decd005b3695095737fd1abb6491cf7cb9cb8f6 Mon Sep 17 00:00:00 2001 From: Marc Jauvin Date: Mon, 6 Jul 2026 12:29:34 -0400 Subject: [PATCH 5/8] add fix/test for potential attach vector found by coderabbit --- .../backend/behaviors/RelationController.php | 17 ++++- .../RelationControllerNestedTest.php | 69 +++++++++++++++++++ 2 files changed, 83 insertions(+), 3 deletions(-) diff --git a/modules/backend/behaviors/RelationController.php b/modules/backend/behaviors/RelationController.php index 477336b313..ab1bb17813 100644 --- a/modules/backend/behaviors/RelationController.php +++ b/modules/backend/behaviors/RelationController.php @@ -666,10 +666,21 @@ protected function resolveModelForHops($root, $hops) ); } - $related = $model->{$relationName}()->getRelated(); - + $relation = $model->{$relationName}(); + $related = $relation->getRelated(); + + // find() on the RELATION instance itself, not a bare + // newQuery() against the related model's blank instance - + // the latter is unscoped and would resolve ANY record with + // a matching id anywhere in the table, not only one that's + // actually a child of $model via this relation. A crafted + // "items[999][taxes]" where item 999 belongs to a + // completely different parent would otherwise still + // resolve successfully, silently bypassing the relation's + // own constraint (the parent foreign key for hasMany, the + // pivot join for belongsToMany). $model = is_numeric($id) - ? ($related->newQuery()->find($id) ?: $related->newInstance()) + ? ($relation->find($id) ?: $related->newInstance()) : $related->newInstance(); } diff --git a/modules/backend/tests/behaviors/RelationControllerNestedTest.php b/modules/backend/tests/behaviors/RelationControllerNestedTest.php index a297b37702..da5068e916 100644 --- a/modules/backend/tests/behaviors/RelationControllerNestedTest.php +++ b/modules/backend/tests/behaviors/RelationControllerNestedTest.php @@ -551,6 +551,75 @@ public function testGetNestedConfigKeyLeavesNonRecursivePathsUnaffected() $this->assertEquals('items[taxes]', $key); } + /** + * Regression for a real, flagged security/correctness issue: + * resolveModelForHops() previously resolved a hop id via a bare, + * unscoped find() against the related model's WHOLE TABLE, rather + * than through the relation itself. That meant a crafted nested + * field referencing an id belonging to a completely different + * parent's record - e.g. another Order's Item - would still + * resolve successfully, silently bypassing the relation's own + * constraint entirely (the parent foreign key for hasMany, the + * pivot join for belongsToMany). Scoping the lookup through + * $model->{$relationName}()->find($id) instead means a hop can + * only ever resolve to a record that's genuinely reachable via + * that specific relation from that specific parent. + */ + public function testResolveModelForHopsCannotCrossIntoADifferentParentsRecord() + { + $orderA = RelationTestOrder::create(['name' => 'Order A']); + RelationTestItem::create(['order_id' => $orderA->id, 'name' => 'Item in Order A']); + + $orderB = RelationTestOrder::create(['name' => 'Order B']); + $itemInOrderB = RelationTestItem::create(['order_id' => $orderB->id, 'name' => 'Item in Order B']); + + $controller = $this->makeController($orderA); + $behavior = $controller->asExtension(RelationController::class); + + // A hop crafted to reference Order B's item, while resolving + // against Order A as the root - a perfectly valid id within + // the items table as a whole, but not a child of Order A. + $result = $this->invokeProtectedMethod($behavior, 'resolveModelForHops', [ + $orderA, + [['items', (string) $itemInOrderB->id]], + ]); + + $this->assertInstanceOf(RelationTestItem::class, $result); + $this->assertFalse( + $result->exists, + 'A hop id belonging to a DIFFERENT parent must not resolve to that record - it should fall back to a blank instance instead' + ); + $this->assertNotEquals( + $itemInOrderB->id, + $result->id, + 'The resolved model must never carry the cross-parent record\'s identity' + ); + } + + /** + * Companion positive-path check for the fix above: a hop id that + * DOES legitimately belong to the current parent must still + * resolve correctly - this is about correctly SCOPING the lookup, + * not breaking it for the ordinary case. + */ + public function testResolveModelForHopsStillResolvesLegitimateChild() + { + $order = RelationTestOrder::create(['name' => 'Order 1']); + $item = RelationTestItem::create(['order_id' => $order->id, 'name' => 'Item A']); + + $controller = $this->makeController($order); + $behavior = $controller->asExtension(RelationController::class); + + $result = $this->invokeProtectedMethod($behavior, 'resolveModelForHops', [ + $order, + [['items', (string) $item->id]], + ]); + + $this->assertInstanceOf(RelationTestItem::class, $result); + $this->assertTrue($result->exists); + $this->assertEquals($item->id, $result->id); + } + // ----------------------------------------------------------------- // Regression: validateField() must prefer an already-active field // over stale POST data, since relationRenderToolbar()/ From 0825c7db6faae00054aaf3049dba5f9607e0e888 Mon Sep 17 00:00:00 2001 From: Marc Jauvin Date: Mon, 6 Jul 2026 12:37:11 -0400 Subject: [PATCH 6/8] fix originalConfig mutation and add regression tests --- .../backend/behaviors/RelationController.php | 25 ++++- .../RelationControllerNestedTest.php | 101 +++++++++++++++++- 2 files changed, 122 insertions(+), 4 deletions(-) diff --git a/modules/backend/behaviors/RelationController.php b/modules/backend/behaviors/RelationController.php index ab1bb17813..0d40923af5 100644 --- a/modules/backend/behaviors/RelationController.php +++ b/modules/backend/behaviors/RelationController.php @@ -715,7 +715,13 @@ protected function aliasNestedConfig($leafField) "(needed for nested field \"{$this->nestedField}\")." ); } - $this->originalConfig[$leafField] = $this->originalConfig[$configKey]; + + // Arrays are value types in PHP - $this->config was + // already an independent copy the moment it was assigned + // from $this->originalConfig at the top of initRelation(), + // so mutating it here can never leak back into + // originalConfig. + $this->config[$leafField] = $this->originalConfig[$configKey]; return; } @@ -726,7 +732,22 @@ protected function aliasNestedConfig($leafField) "(needed for nested field \"{$this->nestedField}\")." ); } - $this->originalConfig->{$leafField} = $this->originalConfig->{$configKey}; + + // Objects are reference types - at this exact point (right + // after initRelation()'s own `$this->config = + // $this->originalConfig;`), $this->config and + // $this->originalConfig are literally the SAME instance, + // not yet narrowed to a single field's config. Mutating + // $this->config directly here would mutate originalConfig + // too, and since originalConfig is never reset/recreated + // after construction, that alias would persist for the + // REST OF THE REQUEST - a later initRelation() call for a + // genuinely root-level field that happens to share this + // bare name would then incorrectly inherit this nested + // field's config instead of its own. Cloning first keeps + // the alias scoped to this call only. + $this->config = clone $this->originalConfig; + $this->config->{$leafField} = $this->originalConfig->{$configKey}; return; } diff --git a/modules/backend/tests/behaviors/RelationControllerNestedTest.php b/modules/backend/tests/behaviors/RelationControllerNestedTest.php index da5068e916..880f34bd8a 100644 --- a/modules/backend/tests/behaviors/RelationControllerNestedTest.php +++ b/modules/backend/tests/behaviors/RelationControllerNestedTest.php @@ -14,11 +14,19 @@ * Self-contained fixture models for nested relation testing. * * Order -(items, hasMany)-> Item -(taxes, belongsToMany)-> Tax - * | - * `-(items, hasMany, self-referencing)-> Item + * | | + * | `-(items, hasMany, self-referencing)-> Item + * `-(taxes, hasMany)-> Tax * * The self-referencing relation on Item is what exercises recursion: * the same field name ("items") appears at two different depths. + * + * Order's OWN "taxes" is deliberately a completely different relation + * (hasMany via order_id, not belongsToMany via the pivot) that just + * happens to share the same bare name as Item's nested "taxes" - this + * is what exercises aliasNestedConfig()'s config-leak fix: resolving + * the nested field must never corrupt the root-level field's config, + * even when both share a name. */ class RelationTestOrder extends Model { @@ -28,6 +36,7 @@ class RelationTestOrder extends Model public $hasMany = [ 'items' => [RelationTestItem::class, 'key' => 'order_id'], + 'taxes' => [RelationTestTax::class, 'key' => 'order_id'], ]; public static function migrateUp(): void @@ -109,6 +118,10 @@ class RelationTestTax extends Model protected $guarded = []; public $timestamps = false; + public $belongsTo = [ + 'order' => [RelationTestOrder::class, 'key' => 'order_id'], + ]; + public $belongsToMany = [ 'items' => [ RelationTestItem::class, @@ -126,6 +139,7 @@ public static function migrateUp(): void Schema::create('backend_test_relation_taxes', function ($table) { $table->increments('id'); + $table->unsignedInteger('order_id')->nullable(); $table->string('label')->nullable(); $table->integer('rate')->nullable(); }); @@ -174,6 +188,24 @@ class RelationTestController extends BackendController ]], ], ], + // Root-level "taxes" - a completely different relation + // (Order's own hasMany, not Item's belongsToMany) that just + // happens to share the same bare field name as items[taxes] + // below. Label is deliberately distinct, so a test can detect + // whether resolving the NESTED "taxes" ever corrupts THIS + // entry's config (see aliasNestedConfig()'s config-leak fix). + 'taxes' => [ + 'label' => 'Order-level Taxes', + 'view' => [ + 'list' => ['columns' => ['label' => ['label' => 'Label']]], + ], + 'manage' => [ + 'form' => ['fields' => [ + 'label' => ['label' => 'Label', 'type' => 'text'], + 'rate' => ['label' => 'Rate', 'type' => 'number'], + ]], + ], + ], 'items[taxes]' => [ 'label' => 'Taxes', 'view' => [ @@ -620,6 +652,71 @@ public function testResolveModelForHopsStillResolvesLegitimateChild() $this->assertEquals($item->id, $result->id); } + /** + * Regression for a config-mutation bug flagged during code review: + * aliasNestedConfig() previously mutated $this->originalConfig + * directly to alias a nested field's config onto its bare leaf + * name. Since originalConfig is the SAME object $this->config is + * reset FROM at the top of every initRelation() call - and is + * itself never reset again after construction - that alias + * persisted for the rest of the request. A LATER initRelation() + * call for a genuinely root-level field sharing the same bare name + * ("taxes" here: Order's own hasMany, completely separate from + * Item's nested belongsToMany) would then incorrectly inherit the + * nested field's config instead of its own. + */ + public function testResolvingNestedFieldDoesNotCorruptRootLevelFieldOfSameName() + { + $order = RelationTestOrder::create(['name' => 'Order 1']); + $item = RelationTestItem::create(['order_id' => $order->id, 'name' => 'Item A']); + + $controller = $this->makeController($order); + $behavior = $controller->asExtension(RelationController::class); + + // Resolve the NESTED "taxes" field first - this is what + // previously mutated originalConfig, since aliasNestedConfig() + // needs to alias "items[taxes]" onto the bare "taxes" key for + // the nested resolution to find its own config at all. + $controller->initRelation($order, "items[{$item->id}][taxes]"); + + // THEN resolve the genuinely root-level "taxes" field, within + // the SAME behavior instance - simulating both being active + // within one request, the same way "items" and "taxes" already + // are elsewhere in this file. + $controller->initRelation($order, 'taxes'); + + $config = $this->readProtectedProperty($behavior, 'config'); + + $this->assertEquals( + 'Order-level Taxes', + $config->label, + 'Resolving the nested "taxes" field must not corrupt the root-level "taxes" field\'s own config' + ); + } + + /** + * Companion check for the opposite order - resolving the root-level + * field first was never actually broken (aliasNestedConfig() only + * ever runs for a nested field in the first place), but confirms + * the fix doesn't accidentally introduce a problem in that + * direction either. + */ + public function testResolvingRootLevelFieldFirstDoesNotAffectLaterNestedResolution() + { + $order = RelationTestOrder::create(['name' => 'Order 1']); + $item = RelationTestItem::create(['order_id' => $order->id, 'name' => 'Item A']); + + $controller = $this->makeController($order); + $behavior = $controller->asExtension(RelationController::class); + + $controller->initRelation($order, 'taxes'); + $controller->initRelation($order, "items[{$item->id}][taxes]"); + + $config = $this->readProtectedProperty($behavior, 'config'); + + $this->assertEquals('Taxes', $config->label); + } + // ----------------------------------------------------------------- // Regression: validateField() must prefer an already-active field // over stale POST data, since relationRenderToolbar()/ From c92f3898c115b0a1333fdf6aace31a05a391b13f Mon Sep 17 00:00:00 2001 From: Marc Jauvin Date: Mon, 6 Jul 2026 14:13:49 -0400 Subject: [PATCH 7/8] add comments to explain the field possible values --- modules/backend/behaviors/RelationController.php | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/modules/backend/behaviors/RelationController.php b/modules/backend/behaviors/RelationController.php index 0d40923af5..c171ee4e79 100644 --- a/modules/backend/behaviors/RelationController.php +++ b/modules/backend/behaviors/RelationController.php @@ -390,12 +390,20 @@ protected function beforeAjax() */ public function initRelation($model, $field = null) { + // No field given explicitly - fall back to whatever was + // posted, if anything. if ($field == null) { $field = post(self::PARAM_FIELD); } $this->config = $this->originalConfig; + // The fallback above didn't find one either: nothing was + // explicitly passed, and none was posted. There's genuinely no + // field to resolve here - no relation type to detect, no + // widgets to build - so store whatever partial state we have + // and return early rather than proceeding into logic that + // assumes a field is present. if ($field == null) { $this->model = $model; $this->field = $field; From 416c7a0b577477eb43b69935feb3d119b5df97a3 Mon Sep 17 00:00:00 2001 From: Meindert <89913092+AIC-BV@users.noreply.github.com> Date: Wed, 26 Aug 2026 17:31:23 +0200 Subject: [PATCH 8/8] Resolve nested relations against the innermost ancestor resolveNestedContext()'s ambient branch trusted the model handed to initRelation() as the relation's parent. That model is $this->model, set from the ENCLOSING relation, because the call arrives as relationRender() -> validateField() -> initRelation($this->model). At one level of nesting the enclosing record is the parent, so this works. At two it is the grandparent, and the leaf relation resolves against the wrong model - in practice a BadMethodCallException, since the grandparent's class usually has no such relation. Walk the ambient path's hops from the root instead, exactly as the reconstructed branch below already does. Both cases now share one resolution mechanism, and the ambient path gains resolveModelForHops()'s $relation->find($id) parent check at every depth. Co-authored-by: Claude Opus 5 --- .../backend/behaviors/RelationController.php | 13 +++-- .../RelationControllerNestedTest.php | 51 +++++++++++++++++++ 2 files changed, 61 insertions(+), 3 deletions(-) diff --git a/modules/backend/behaviors/RelationController.php b/modules/backend/behaviors/RelationController.php index c171ee4e79..ae095fc8fd 100644 --- a/modules/backend/behaviors/RelationController.php +++ b/modules/backend/behaviors/RelationController.php @@ -536,8 +536,13 @@ protected function isNestedField() * a) AMBIENT - the nesting stack is non-empty, meaning a manage * form is actively rendering right now (see * relationMakePartial()) and this field is nested directly - * under whatever's on top of it. $model is trusted as-is - it's - * the widget's own bound model. + * under whatever's on top of it. $model canNOT be trusted + * here: initRelation() is reached as + * relationRender() -> validateField() -> initRelation($this->model), + * and $this->model is the record of the ENCLOSING relation, + * not of the manage form actually rendering. Those coincide at + * one level of nesting and diverge at two, so the parent is + * walked from the root exactly as in (b). * b) RECONSTRUCTED - $field is already a bracket path (e.g. * "items[5][taxes]"), walked from the root to find the actual * model. This is the common case for anything other than an @@ -574,7 +579,9 @@ protected function resolveNestedContext($model, $field) if ($ambientPath !== null) { $this->nestedField = $ambientPath . '[' . $field . ']'; - return [$model, $field, true]; + [$hops] = $this->parseBracketField($this->nestedField); + + return [$this->resolveModelForHops($this->rootModel, $hops), $field, true]; } if (strpos($field, '[') !== false) { diff --git a/modules/backend/tests/behaviors/RelationControllerNestedTest.php b/modules/backend/tests/behaviors/RelationControllerNestedTest.php index 880f34bd8a..4feea5b362 100644 --- a/modules/backend/tests/behaviors/RelationControllerNestedTest.php +++ b/modules/backend/tests/behaviors/RelationControllerNestedTest.php @@ -918,6 +918,57 @@ public function testAmbientFieldDistinguishesBetweenSiblingItems() $this->assertNotEquals($nestedFieldForA, $nestedFieldForB, 'Two sibling items\' ambient contexts must resolve to distinct qualified fields'); } + /** + * Two levels of ambient nesting - an Item's manage form rendering + * a sub-item's manage form, which itself renders a relation field. + * + * The model RelationManager hands initRelation() is always + * $this->model, i.e. the record of the ENCLOSING relation. At one + * level of nesting that IS the parent, so trusting it works. At + * two it is the grandparent, and trusting it resolves the leaf + * relation against the wrong ancestor - in the real world a + * BadMethodCallException, since the grandparent's class usually + * has no such relation at all. + */ + public function testAmbientFieldTwoLevelsDeepResolvesInnermostAncestorNotEnclosingRecord() + { + $order = RelationTestOrder::create(['name' => 'Order 1']); + $itemA = RelationTestItem::create(['order_id' => $order->id, 'name' => 'Item A']); + $subItem = RelationTestItem::create(['parent_item_id' => $itemA->id, 'name' => 'Sub A1']); + + $controller = $this->makeController($order); + $behavior = $controller->asExtension(RelationController::class); + $controller->initRelation($order, 'items'); + + // Item A's manage form is rendering; its sub-items field + // resolves ambiently against it. + $this->pushNestingStack($behavior, "items[{$itemA->id}]"); + $controller->initRelation($itemA, 'items'); + + // The sub-item's own manage form is now rendering INSIDE Item + // A's, and a relation field within it resolves ambiently. + $this->pushNestingStack($behavior, "items[{$itemA->id}][items][{$subItem->id}]"); + + // Item A, not the sub-item: exactly what + // validateField() -> initRelation($this->model) passes here. + $controller->initRelation($itemA, 'taxes'); + + $this->popNestingStack($behavior); + $this->popNestingStack($behavior); + + $this->assertEquals( + "items[{$itemA->id}][items][{$subItem->id}][taxes]", + $this->readProtectedProperty($behavior, 'nestedField'), + 'Two levels of ambient nesting should qualify the field against the whole stack path' + ); + + $this->assertEquals( + $subItem->id, + $this->readProtectedProperty($behavior, 'model')->id, + 'A field nested two levels deep must resolve against the innermost ancestor, not the enclosing record' + ); + } + /** * Narrower test for relationMakePartial() specifically - confirms * it actually pushes before rendering and pops after, regardless