Follow-up to #3590444, which fixed the `::handle` branch of `EndpointController::access()`. The **second** path through the same method has the same class of defect, and in one respect a worse one. ## Which path `EndpointController::access()` has two outcomes. The first, fixed in #3590444, is the early return taken when the current route *is* the endpoint route being served: it grants access so that `::handle` can decide, and now carries `mergeCacheMaxAge(0)`. Everything else falls through to the bottom of the method, which dispatches `eca_endpoint:access` and returns the model's verdict: ```php $forbidden = AccessResult::forbidden("No ECA configuration set an access result"); $event = $this->triggerEvent->dispatchFromPlugin('eca_endpoint:access', $path_arguments, $account, $forbidden); if ($event instanceof AccessEventInterface && ($result = $event->getAccessResult())) { return $result; } return $forbidden; ``` This is the path taken when the route is not the endpoint route at all - the "fake" route match used while building local menu links, `Url::access()`, link generation - or when it *is* the endpoint route but for different arguments than the ones being served, for example a link on one endpoint page pointing at another. It is the path added by #3535444 for menu link support, and it is the path every existing assertion in `EndpointControllerTest` exercises. ## The defect The verdict returned here carries **no cache contexts, no cache tags and `Cache::PERMANENT`**. `SetAccessResult::execute()` (`modules/access/src/Plugin/Action/SetAccessResult.php`) builds a bare `AccessResult::allowed()`, `neutral()` or `forbidden()` and hands it to `EndpointAccessEvent::setAccessResult()`, which only `orIf()`s them together. When no model reacts, the fallback `$forbidden` above is returned instead. Either way nothing along the chain attaches cacheability. So where #3590444 was "cacheable forever, varying by four contexts", this is **cacheable forever, varying by nothing**. A verdict computed for one user on one path can be replayed for every user on every path, and enabling or editing an ECA model does not invalidate it, because there is no `config:eca.eca.*` cache tag anywhere in the chain. ## Three distinct gaps, not one Measured against `eca_access`, which solves the same problem of a model deciding access (`modules/access/src/Hook/AccessHooks.php`, lines 50-53, 84-87 and 120-123), this path omits both halves of that solution plus a third thing: 1. No `RenderContext` wrapper, so cacheability that ECA raises during token replacement is either dropped or bubbles into whatever ambient render context happens to be active and gets attributed to the wrong thing. 2. No `mergeCacheMaxAge(0)`. 3. No route at all from the model to the result's cacheability. `eca_access_set_result` has no way to declare "this verdict depends on X". `eca_access` gets away with that because max age 0 makes it moot. ## Why this was not folded into #3590444 Deliberately, for reasons that are also the reason this needs discussion rather than a patch. **The blast radius is different and material.** The branch fixed in #3590444 sits on a route whose response `::handle` already forces private and uncacheable, so max age 0 there costs nothing. Applying max age 0 here makes every menu link pointing at an ECA endpoint permanently uncacheable, and that bubbles up through `DefaultMenuLinkTreeManipulators::checkAccess()` into the menu block, the page render cache and Dynamic Page Cache. That is a real performance regression for exactly the feature #3535444 added. **Gap 3 cannot be closed by editing this method.** It needs a decision about `eca_access_set_result` and `AccessEventInterface`, which is a design change. So the right answer here may well not be max age 0 at all, but giving models a way to declare their own cacheability - with max age 0 as the fallback when they declare none. That is worth deciding before anyone writes code. ## Proposed resolution Decide between: 1. **Match `eca_access`**: wrap the dispatch in a render context, `addCacheableDependency()` the result, `mergeCacheMaxAge(0)`. Correct, consistent, and costs menu link cacheability site-wide. 2. **Let models declare cacheability**, defaulting to max age 0 when they do not. Preserves cacheability for models whose verdict really is static, at the price of a new API surface on `eca_access_set_result` and `AccessEventInterface`. Either way the result should also carry the `config:eca.eca.*` cache tags of the models that reacted, so that enabling, editing or disabling a model invalidates a cached verdict. That part looks uncontroversial regardless of which option is chosen. ## Remaining tasks - Decide between option 1 and option 2. - Implement, with kernel coverage. Note that a test asserting on this path is easy to write today, because `KernelTestBase` pushes a request with no route object, so `access()` falls through to exactly this branch. - A short comment on the `::handle` branch pointing here, so the next reader does not mistake the asymmetry between the two branches for an oversight. ## Backport `access()` is byte-identical on `3.1.x`, `3.0.x` and `2.1.x`, and `eca_access`'s hooks exist on all three, so whatever is decided applies to all three branches. The backport risk is not equal to #3590444's, though: a one-line max age change is safe to backport, a menu link cacheability change on a stable branch is not. --- AI-Generated: Yes (agent found this while implementing #3590444, traced the result object from `SetAccessResult::execute()` through `EndpointAccessEvent` to the return, verified the `eca_access` contrast and the byte-identical state of the method on all three branches, and deliberately kept it out of that MR rather than expanding its scope).