Allow patching a multiselect to empty - #2018
Closed
xIrusux wants to merge 67 commits into
Closed
Conversation
🔄 synced file(s) with pimcore/platform-version
* Rename new-docs.yml to docs.yml * Remove new-docs.yml after rename to docs.yml * Rename new-static-analysis.yaml to static-analysis.yaml * Remove new-static-analysis.yaml after rename to static-analysis.yaml
* [Workflow] Add WorkflowFilter to scope element grids by workflow place (#1938) * [Workflow] Exclude folders from workflow element queries (#1930) * [Workflow] exclude folders from workflow element queries Folders share their element's ctype (asset/object/document) in element_workflow_state, so fetchByWorkflowState listed asset folders alongside real assets in the workflow pending-items widget and the workflow_get_elements click-through list (pimcore/studio-dashboards-bundle#301). Filter out folder subtypes in SQL via the already-present assets/objects/ documents joins. The IS NULL guard per table is required: a plain "type != 'folder'" would drop every non-matching LEFT JOIN row (NULL type) through three-valued logic, emptying the result. Orphaned states (element deleted) are preserved unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Address static-analysis finding: drop unused leftJoin callback params Verify the join count via expects(exactly(3)) instead of a capturing callback with unused $fromAlias/$join parameters (Codacy/PHPMD UnusedFormalParameter). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * [Workflow] add WorkflowFilter to scope element grids by workflow place New search-index filter (type `workflow`, key=workflow name, value=place) that resolves the matching element ids via WorkflowElementsRepository (folders already excluded) and applies the standard searchByIds modifier. Lets the native element listing be filtered to a workflow state server-side — the basis for the state distribution donut drill-down opening the native grid instead of a bespoke list. Element type is derived from the query (data-object / asset / document). Empty id set matches nothing; a MAX_IDS ceiling guards the OpenSearch terms limit (logged). Registered with the pimcore.studio_backend.search_index.filter tag. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * [Workflow] document the workflow grid filter Add the `workflow` column filter to the Grid architecture docs (filters table + example): key = workflow name, value = place (omit/null = all states), folders excluded, ids resolved server-side. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Apply php-cs-fixer changes --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…public/default 🔄 synced file(s) with pimcore/workflows-collection-public
Co-Authored-By: Claude <noreply@anthropic.com>
Static analysis needs no secrets: the shared reusable has no cache:clear/ kernel-boot step, phpstan.neon sets no containerXmlPath, composer.json has no auto-scripts, and the repo is public (deps from public Packagist). So pull_request_target only adds risk — it currently checks out fork PR head with secrets in scope, which actions/checkout now refuses (pwn-request guardrail), breaking fork PRs at Checkout code. Switching to pull_request runs fork PRs safely without secrets and fixes that failure with no security gate needed.
…public/default 🔄 synced file(s) with pimcore/workflows-collection-public
…ull_request Run static analysis on pull_request instead of pull_request_target
* Add PR Guardrails trigger workflow (platform-version#226) * Pass only the three guardrail tokens instead of secrets: inherit
Add a `pimcore_studio_backend.translations.auto_create_missing_keys` config flag (default: true) and expose it to the frontend via SystemSettingsProvider as `auto_create_translations`. This lets projects that maintain the `studio` translation domain purely through YAML files opt out of the frontend-driven auto-creation of missing keys, which otherwise pollutes the `translations_studio` table with non-translatable labels (numeric values, select-field choices). The flag defaults to true, preserving the existing behaviour. The frontend gate that consumes this flag ships separately in studio-ui-bundle. Manual creation via the Translations editor is unaffected. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…e-translation-keys Allow disabling auto-create of missing translation keys
…public/default 🔄 synced file(s) with pimcore/workflows-collection-public
…em columns (#1966) * [Grid] Add User Modification and User Owner as selectable object system columns Introduces a new `system.user` grid column type (definition + resolver) and exposes `userModification` / `userOwner` as selectable system columns for data objects. The resolver returns the raw user id; the Studio UI resolves the username for display. Refs pimcore/studio-ui-bundle#1956 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * [Grid] Honor isFilterable() for system columns; keep user columns non-filterable SystemFieldCollector::overrideFilterable() used $definition->isSortable() in its default branch, so a definition's isFilterable() was ignored. This advertised the new userModification/userOwner columns as filterable despite UserDefinition declaring them non-filterable. Use isFilterable() instead — every existing data object system column has isSortable() === isFilterable(), so only the user columns change (sortable, not filterable), matching the intended contract. Adds a UserDefinition unit test locking the sortable/non-filterable flags. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Fix formatting of exception documentation in UserResolver * Fix formatting and add missing docblock for resolve method --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Expose version coauthor fields in version listing schema Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SsEa2MTm2gRPyGEdHGnnoF * Accept coauthor fields on element save and patch payloads Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SsEa2MTm2gRPyGEdHGnnoF * Allow setting version coauthor via PUT /versions/{id} Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SsEa2MTm2gRPyGEdHGnnoF * Treat empty strings as clear when updating version coauthor Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SsEa2MTm2gRPyGEdHGnnoF * Require pimcore/pimcore ^2026.3 for version coauthor support Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SsEa2MTm2gRPyGEdHGnnoF * Address Codacy findings on coauthor changes Drop the redundant "(e.g. agent)" concat from the coauthorType OpenAPI descriptions (the Property already carries example: 'agent') and shorten the previousCoauthorContext locals to coauthorSnapshot, keeping both the line length and PHPMD's variable-length rule happy. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SsEa2MTm2gRPyGEdHGnnoF * Add UpdateService coauthor context tests Cover UpdateService::update() coauthor-context wiring end to end through the public API with a real Pimcore\Model\Version\CoauthorContext: payload keys activate the context only for the duration of the save, empty/partial keys never activate it, an outer context is overridden during save and restored exactly afterwards, and the context is still restored when save() throws. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SsEa2MTm2gRPyGEdHGnnoF * Resolve merge conflicts and address review feedback The merge of 2026.x was pushed with unresolved conflict markers in UpdateService and PatchService, leaving both classes unparsable. - Resolve both conflicts keeping all additions from either side. In patchElement() the permission checks run before the coauthor context is set up, so a ForbiddenException cannot leave context state behind. - Replace the snapshot/restore helpers duplicated across both services with the core CoauthorContextInterface::withCoauthor() scope helper, extracted into CoauthorContextTrait as the single owner of payload coauthor handling. - Reject coauthorType/coauthor values exceeding the widths of the core versions columns (50/255) instead of letting the save truncate them: a 422 on the update/patch payloads and a Length constraint on PUT /versions/{id}. The limits are declared once in VersionCoauthor and surfaced in OpenAPI via the new CoauthorType/Coauthor property attributes, which also de-duplicate the five request bodies. - Cover the patch path, the length limits and the version update parameter, which previously had no tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SsEa2MTm2gRPyGEdHGnnoF * Move payload coauthor handling into a dedicated service Injecting the coauthor context plus a trait pushed PatchService to 21 class dependencies (php:S1200, max 20). An Element service replaces both, so each caller depends on one collaborator instead of two and the class drops back under the limit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SsEa2MTm2gRPyGEdHGnnoF --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: fashxp <8792145+fashxp@users.noreply.github.com>
* [Asset List] Add file size field filter (#705) Make the file-size system column filterable and add a dedicated FileSizeFilter search-index handler. Following the quantity-value convention, the handler reads the client value plus unit (KB/MB/GB), converts it to bytes and applies it as a numeric range on the `fileSize` field. The "is" setting matches a one-unit-wide band (mirroring the datetime roundToDay behaviour), because file size is stored byte-precise and an exact match would practically never hit. Co-Authored-By: Claude <noreply@anthropic.com> * [Asset List] Tolerate a single "between" bound in the file size filter (#705) Treat a "between" filter with only one bound set as an open-ended range instead of throwing, matching the numeric filter's leniency. Co-Authored-By: Claude <noreply@anthropic.com> * [Asset List] Fix file size "is" band boundaries and add filter tests (#705) - filterNumberRange bounds are exclusive (gt/lt), so widen the "is" band by one byte on each side; otherwise a file of exactly N units never matched "is N" - Add FileSizeFilterTest covering KB/MB/GB conversion, all four settings, the inclusive "is" band boundaries, single-bound "between", and malformed unit/value payloads Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: Claude <noreply@anthropic.com>
* Cover MCP endpoints with the general Studio API rate limiter
RateLimitSubscriber guarded on isStudioBackendPath($path, $urlPrefix), where
url_prefix defaults to /pimcore-studio/api. The MCP firewall serves
/pimcore-mcp/, a separate path space, so no MCP endpoint was covered by the
studio_api_general limiter at all - neither the Studio MCP routes nor the
Pimcore Agent bundle's /pimcore-mcp/agent/{group}.
Adds an explicit prefix branch. The prefix is matched with a trailing slash so
sibling routes that merely start with the same characters are not swallowed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014zK7dwM71xcjbp675aiAEQ
* Defer PAT user lookup into the UserBadge loader
authenticate() resolved the username, loaded the Pimcore user, validated it and
only then built the UserBadge - throwing at every failure step. Because
AuthenticatorManager::executeAuthenticator() dispatches CheckPassportEvent only
after authenticate() returns, an authenticator that throws never produces a
passport, so LoginThrottlingListener::checkPassport() (priority 2080) can never
block it. Only LoginFailureEvent still fires, which means failures would fill
the limiter while nothing is ever rejected - and the sole party who can be
locked out is the holder of a valid credential.
resolveUsername() reads an in-memory token map, so the identifier is available
with no database work. Building the badge first and moving the lookup into its
loader makes the standard firewall throttling usable, and as a side effect the
database is no longer touched during authenticate().
Credentials that resolve to no user share one constant identifier: a per-token
identifier would hand every guess its own local bucket, leaving only the global
per-IP tier doing any work.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014zK7dwM71xcjbp675aiAEQ
* Defer MCP access token lookup into the UserBadge loader
Same root cause as the preceding PAT change: authenticate() validated the token
and threw on failure, so no passport was ever produced and
LoginThrottlingListener::checkPassport() could not block the request.
validate() is itself the lookup, so unlike PAT no real identifier exists before
it runs; the placeholder identifier is used throughout and the global per-IP
limiter tier does the work here.
The _mcp_token_reference binding moves into the badge loader with the lookup.
It still lands before the controller - Symfony resolves the badge during
CheckPassportEvent - and is now set only for a token that actually validated,
which a new test pins.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014zK7dwM71xcjbp675aiAEQ
* Answer throttled MCP clients with 429 and Retry-After
login_throttling raises TooManyLoginAttemptsAuthenticationException, an ordinary
AuthenticationException, so the firewall entry point would render it as a bare
401 with no Retry-After - telling an MCP client its credential is wrong when the
truth is "back off". MCP clients are programs that act on Retry-After.
A shared trait maps it to 429 and converts the exception's threshold, which is
reported in minutes, into seconds. Ordinary failures still return null, so the
existing fall-through to the next authenticator on the firewall is unchanged.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014zK7dwM71xcjbp675aiAEQ
* Throttle failed MCP authentication attempts
Enables Symfony's standard login_throttling on the pimcore_mcp firewall: 5
failures per 5 minutes per identifier+IP, plus an automatically derived global
limiter at 25 per 5 minutes per IP. Until now the MCP firewall had no
brute-force protection of any kind, and the general Studio API limiter did not
reach it either.
Deliberately sequenced after the authenticator restructure. Enabled against
authenticators that throw inside authenticate(), throttling would have been
worse than absent: LoginFailureEvent fires and fills the limiter, but
CheckPassportEvent never does, so guesses are never blocked while the holder of
a valid credential is the only party who can be locked out.
mcp_firewall_settings is consumed by host applications as
`pimcore_mcp: '%pimcore_studio_backend.mcp_firewall_settings%'`, so this reaches
every installation without an application-side change - including the Pimcore
Agent bundle's /pimcore-mcp/agent/{group} endpoints.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014zK7dwM71xcjbp675aiAEQ
* Document MCP authentication throttling
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014zK7dwM71xcjbp675aiAEQ
* Note that blocked MCP requests still count against the limiter
Observed during end-to-end verification: a throttled request still reaches
LoginFailureEvent, so onFailedLogin() consumes another token. A client that
polls while blocked therefore keeps its own window full indefinitely. This is
stock Symfony behaviour and desirable against an attacker, but clients need to
know to honour Retry-After rather than retry-loop.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014zK7dwM71xcjbp675aiAEQ
* Key dynamic MCP tokens per token instead of one shared bucket
Review finding: every dynamic token, valid ones included, entered throttling
under the same placeholder identifier, so five wrong pmcp_ guesses exhausted the
local identifier+IP bucket and the next legitimate dynamic token from that IP was
rejected before validation. The earlier end-to-end check missed this because it
used a static PAT, which resolves to a real username and therefore has its own
bucket.
Dynamic tokens now key on a non-secret digest of the token itself. Repeating one
wrong token is still caught by the local tier; sweeping distinct tokens now lands
in a fresh local bucket each time and is bounded by the global per-IP tier, which
is the looser but correct trade - a guesser can no longer evict a legitimate
holder. Verified end-to-end: 6 distinct guesses no longer lock out at 5, the same
token repeated 6 times still yields 429, and a valid credential still passes.
Adds an integration-style regression driving the real LoginThrottlingListener
against the real DefaultLoginRateLimiter, which reproduces the original defect.
Also corrects two documentation claims:
- success does NOT reset the budget. LoginThrottlingListener skips the reset
for peekable limiters, and DefaultLoginRateLimiter is peekable, so failures
decay only when the fixed window rolls over.
- the global tier is per-IP and shared, so a sustained attack can still
throttle other clients behind the same address.
INVALID_IDENTIFIER returns to private now that no other class consumes it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014zK7dwM71xcjbp675aiAEQ
* Correct the MCP firewall override instructions
The documented override told integrators to write login_throttling under
security.firewalls.pimcore_mcp. That firewall is declared as a whole-value
parameter substitution - pimcore_mcp: '%pimcore_studio_backend.mcp_firewall_settings%'
- so a partial block there does not merge: it replaces the entire firewall
definition and takes the pattern, provider and custom authenticators with it.
Following the old snippet would have disabled MCP authentication rather than
retuned it.
The real extension point is the parameter itself, which the bundle sets only
when it is not already defined. Verified against a running application: an
application-supplied parameter survives with its own max_attempts and interval.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014zK7dwM71xcjbp675aiAEQ
* Restore PatAuthenticator's 401 and harden the throttling response
Independent review found that this branch had silently dropped a security
behaviour. PatAuthenticator is the last authenticator on the MCP firewall - the
other two return null to fall through to it - and it previously answered every
failure with a 401 JsonResponse. Routing its failure handler through the
throttling trait made it return null for any non-throttling failure, so an
invalid PAT no longer produced a response at all and the request continued
unauthenticated, leaving the 401 to whatever role check the consuming bundle
happens to declare. Restored, with a regression test that fails without it.
Also from review:
- Retry-After could be 0. The threshold is ceil((reset - now) / 60) and is 0
at a window boundary, which advertised an immediate retry. Floored to one
minute.
- The dynamic-token throttle identifier was a truncation of the very SHA-256
value McpAccessTokenService stores as the token's server-side verifier, and
it is written to the request's LAST_USERNAME attribute. Domain-separated so
the digest is unrelated to the stored verifier.
- Use the bundle's HttpResponseCodes enum rather than Response::HTTP_*
constants, which appear once in src/ against ~1189 uses of the enum.
- Corrected the claim that retrying while throttled extends the lockout. The
fixed-window limiter declines an over-limit request without recording it;
what actually happens is that both tiers are consumed on every attempt, so a
locally-blocked client keeps draining the shared global per-IP budget.
- Corrected the authenticator-chain description, which claimed all three
return null on failure.
- Test fixes: a tautological assertTrue(true), and (bool) casts that hid the
difference between supports() returning null and false.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014zK7dwM71xcjbp675aiAEQ
* Narrow MCP throttling to guessed PATs and split the MCP rate limiter
The throttling added earlier keyed dynamic pmcp_ tokens per token and leaned on
Symfony's derived per-IP tier for anything a per-token bucket could not catch.
That aimed at the wrong threat and paid for it with collateral: a pmcp_ token is
32 random bytes, so guessing one is not a reachable attack and a counter adds
nothing, while the per-IP tier is peeked by every client on an address - so
guesses could push an unrelated valid credential into a 429.
Only static PATs are guessable, and PatAuthenticator already handles them well:
an unrecognised token resolves to a shared placeholder identifier, which is a
per-IP failed-guess counter in all but name. That is kept and becomes the whole
mechanism.
- McpLoginRateLimiter replaces DefaultLoginRateLimiter via login_throttling.limiter.
It has one tier and hands out a bucket only for the placeholder identifier, so a
credential that resolves to a user is exempt from both the block and the count.
A successful authentication cannot be rejected because no bucket exists to
reject it from - a property of the keying, not a threshold.
- McpAccessTokenAuthenticator and SessionBridgeAuthenticator go back to their
previous shape, and McpThrottlingResponseTrait folds into its only remaining
user. The 429 + Retry-After mapping stays on PatAuthenticator.
- MCP request volume moves off studio_api_general onto studio_mcp_general
(3000/min per IP). The Studio number is sized for a browser UI, while a single
agent server can serve every chat in an installation from one address, so the
two budgets should not share a bucket.
- ApiExceptionSubscriber now covers /pimcore-mcp/ as well, or the 429 that
RateLimitSubscriber newly raises there would miss the Studio JSON envelope.
Co-Authored-By: Claude <noreply@anthropic.com>
* Catch the expected throttling in the guess loop
testAValidCredentialIsNotBlockedByGuessesFromTheSameIp drives twenty guesses to
show they cannot reach a valid credential, but from the sixth onwards the
attacker is blocked and checkPassport() throws - which failed the test rather
than the assertion it was setting up.
Co-Authored-By: Claude <noreply@anthropic.com>
* Correct the limiter wiring sentence and list the new MCP limiters
The MCP page had the substitution backwards: login_throttling.limiter is how
McpLoginRateLimiter is supplied, not how Symfony's default arrives - the default
is what Symfony builds when that option is absent.
04_Rate_Limiting.md was missed entirely. It is the reference table for every
limiter the bundle ships, and studio_mcp_general and studio_mcp_login were not
in it. Both are listed now, along with two distinctions that are easy to get
wrong: MCP paths use studio_mcp_general instead of studio_api_general rather
than on top of it, and rate_limiting.enabled: false does not reach
studio_mcp_login, which lives on the firewall rather than in the subscriber.
Co-Authored-By: Claude <noreply@anthropic.com>
* Scope the exception subscriber to the 429 and correct the throttling claim
Review findings on the previous commits.
ApiExceptionSubscriber claimed every HttpException raised on /pimcore-mcp/, not
just the one this bundle raises there. MCP is JSON-RPC and owns its error shapes,
so a 404 from the MCP server was being rewritten into the Studio envelope. Only
RateLimitException is claimed now, and ApiExceptionSubscriberTest covers all four
combinations - the class had no test at all before.
The "a successful authentication is never throttled" claim was too strong, and a
reproduction against the real AuthenticatorManager shows why: the manager keeps
running authenticators after one has already succeeded, so a request carrying a
valid session cookie *and* an unrecognised PAT is still judged on the PAT and can
be answered with the 429. Such a request already failed before throttling existed
- PatAuthenticator owns the terminal response and answered 401 - so the status
code is what changed, not whether it works. The guarantee is stated per credential
now, in both the interface docblock and the docs.
The unknown-credential sentinel gains a leading NUL. PatAuthenticator takes the
identifier straight from the configured token map, so a plain "__invalid__" would
collide with a Pimcore user of that name and pull their valid PAT into the guess
bucket; no username can contain a NUL.
Also: import order in PatAuthenticatorTest (ordered_imports covers tests/), and a
missing overflow test for the MCP request limiter.
Co-Authored-By: Claude <noreply@anthropic.com>
* Trim the MCP throttling docs to what a reader needs
The section had grown to explain why the design is the way it is: the tier
mechanics of the limiter it replaces, when a fixed window resets, why retrying
does not extend a lockout, which login_throttling keys go unread. That is
implementation rationale, and the code comments already carry it.
What is left is what a reader has to act on - the rate and the response, which
credentials are counted and which are not, the one-credential-per-request rule,
trusted proxies, and the two config blocks to copy.
Co-Authored-By: Claude <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
#1972) * Add the shared MCP tool boundary: error handler and schema normalizers This bundle already owns the MCP infrastructure every tool-hosting bundle depends on: the firewall pattern, the access-token entity, repository, service and GC task, the three authenticators, and the PSR-7 bridge in config/mcp.yaml. It owns no tools, which is exactly why the boundary belongs here rather than in any one bundle that has them. Three classes have been developed and hardened in pimcore-agent-bundle and are moved here so they stop being copied. `McpToolErrorHandler` and `ObjectParameterNormalizer` already exist, separately and by now divergently, in pimcore-agent-bundle, copilot-bundle and data-hub-simple-rest (126, 126 and 268 lines). `ToolInputSchemaNormalizer` exists only in the agent bundle, so the other two never got its fix systemically and data-hub-simple-rest widened the affected parameters by hand instead. None of the three needs mcp/sdk, so this adds no dependency: the handler needs PSR-3, and both normalizers are array in, array out. What they do: - `McpToolErrorHandler` is the terminal `catch (Throwable)` of an MCP tool. Tool results leave the Pimcore boundary — external clients forward them to whichever third-party model they are wired to — so it forwards no exception message at all, logs the exception in full, and answers with a generic sentence plus a correlation id. Note that Symfony does not do this for you: a tool catches its own exception and returns the text as application data inside an HTTP 200 JSON-RPC result, so kernel.debug and the production error page never see it. - `InvalidMcpToolArgumentException` is the one type it forwards. Throwing it is an explicit statement that the message was composed for the caller out of the caller's own input. - `ToolInputSchemaNormalizer` and `ObjectParameterNormalizer` are two halves of one mechanism: a #[Schema] attribute pins a parameter to a single type, contradicting the `default: null` the SDK still emits, so the schema has to be widened to accept null and `{}`; widening to admit `array` also admits a populated list, which the second half rejects. There is deliberately no allowlist of "client-safe" exception types. Safety is a property of the construction, not of the class: in this bundle alone `ValidationFailedException` is given a caller-facing literal at five call sites and an inner getMessage() at CloneService and WidgetValidationService, so any class-level marking of it is wrong half the time. Where a foreign message really is worth forwarding, the tool type-catches it and says why at the catch. Additive only; no existing behaviour changes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Sra46591DFmHjHQag4fCoT * Address review: unresolvable @see refs, entropy failure at the boundary Three points from the Copilot review. `@see McpToolErrorHandler` and `@see ObjectParameterNormalizer` in the exception docblock resolved against `Mcp\Exception`, where neither class exists, so generated docs and IDE navigation pointed nowhere. Imported. `random_bytes()` can throw on entropy exhaustion, and it was called before the log write in the terminal error boundary. That is the worst possible place for a throw: the tool's original exception would be neither logged nor converted, and a `RandomException` would escape in its place. The id now comes from `correlationId()`, which falls back to a hash of `hrtime()` — monotonic, non-blocking, cannot throw. `random_bytes()` stays the primary source only because it is the one that does not raise Sonar's weak-randomness hotspot; a correlation id is not a secret and only has to be greppable. The numeric-key case is real and is now documented rather than silently accepted. `{"0":"a","1":"b"}` is rejected even though the schema says `object`, but that is not a choice this class can make differently: `Mcp\JsonRpc\MessageFactory` decodes the payload with `json_decode($input, true)` before any tool is reached, PHP casts numeric string keys to integers, and the two shapes decode to *identical* values — `===` returns true. No downstream inspection can separate them. The class docblock now states the contract, names the escape hatch (any non-integer key, or integer keys not sequential from zero), and says which parameters should not use this guard at all. Tests cover all three: the decode identity is asserted alongside the rejection, so the test fails if that upstream behaviour ever changes and the limitation could be lifted; plus the mixed-key acceptance and correlation-id well-formedness. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Sra46591DFmHjHQag4fCoT --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
* Add notification subscription framework Introduces a generic, extensible notification framework: bundles contribute notification *types* and delivery *channels* as tagged services, and each user chooses per type whether they are notified and through which channels. The immediate win needs no contributing bundle at all. Every notification Pimcore writes today is untyped, so it falls into a built-in catch-all type whose pop-up preference is honoured when the notification is published over Mercure. That turns today's unconditional toasting into a choice for workflow transitions, user-to-user messages and anything a bundle writes directly, without touching a single producer. Design notes worth keeping in mind when extending this: - A type declares only *whether* it may leave the application, never through which channel. Supported channels are derived from that capability, so a bundle contributing a Teams channel lights up for existing types without those bundles being edited. - The pop-up is modelled as a channel from the user's point of view but is not a transport: it is a preference read at publish time. Storing it in the same JSON set is what keeps the schema stable when channels are added. - No channel implementation ships here. The only type present is the catch-all, which deliberately allows no external delivery — a bucket of unclassified notifications is not something to email. Whichever bundle first contributes an externally-deliverable type contributes the channel alongside it. - Type ids are capped at 20 characters because notifications.type is VARCHAR(20) and MySQL truncates silently outside strict mode. The registry rejects violations at boot rather than letting a truncated id match nothing. - The catch-all reports a different label when it is the only registered type: there is nothing for it to be "everything else" to. NotificationMinimal gains popup and payload. Both are additive and popup defaults to true, so a client that has not adopted them behaves as before. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Always register the notification catch-all type The catch-all was expected to arrive through the service tag like any other descriptor. When the tag was not applied the registry held nothing, so a bare installation reported no subscribable types and the preferences screen came up empty — found by calling the endpoint against a running app. Registering it directly is also the better design regardless of the tag: every notification ever written falls into this type, and on an installation with no contributing bundle it is the only one there is. Its presence should not be something wiring can break. Also fixes the channel translation key prefix, which did not match the keys shipped in studio-ui. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Mark the framework's internal contracts @internal The module marks its internal service, repository and hydrator interfaces @internal; the new internal contracts were missing it. Adds it to the internal registry/subscription interfaces and the internal EffectiveSubscription value object, so the public surface stays limited to what is genuinely meant for external use — the descriptor and channel interfaces, the dispatcher, the DispatchableNotification producers build, the subscription-collection event, and the API schemas — all of which deliberately keep no @internal. Docblock only; no behaviour change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Tag notification descriptors and channels via a compiler pass The framework's extensibility relies on bundles contributing type descriptors and delivery channels as tagged services, collected by the registries' tagged iterators. That tagging was expressed as #[AutoconfigureTag] on the interfaces — which, it turns out, does not tag implementers in Pimcore's container (the existing tagged collectors here, e.g. GDPR providers, are all tagged explicitly in YAML). The result was that no contributed descriptor or channel was ever collected: the type registry only ever saw the built-in catch-all, which it adds directly. Surfaced while wiring collab-bundle's notification types: they registered cleanly but never appeared. A compiler pass tags every implementer of the descriptor and channel interfaces. It runs after all bundle extensions load, so a type or channel from any bundle is picked up without that bundle knowing the tag name — which is what makes the framework actually extensible. Idempotent, so a bundle that tags explicitly is not tagged twice, and abstract definitions are skipped. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Add an email delivery channel for notifications The framework shipped the channel seam but no transport. This adds EmailChannel, the first ChannelInterface implementation, so externally-deliverable types (the Collab types today) can reach people by email as well as the bell and pop-up. - EmailChannel resolves the recipient, language and an absolute deep link inside the producing request, then hands a fully-resolved SendNotificationEmailMessage to the pimcore_core transport. The blocking send happens in the worker, so a slow mail server never delays the comment or assignment that triggered it. - The email mirrors the bell entry — the notification's own title and message plus a link, nothing from the payload — except one navigation hint: a producer may supply an app-relative deepLink (host-relative only, so a payload can never make the button off-site) to point at a better destination than the linked element, e.g. a Collab task or discussion in its Overview. - The body is a Twig template rendered in the recipient's language. It is overridable: point notifications.email.template at your own template, or drop a file at templates/bundles/PimcoreStudioBackendBundle/notification/email.html.twig. - Delivery rides the existing pimcore_core messenger transport (routing registered in the bundle extension), so the standard messenger:consume worker covers it. Registering EmailChannel makes the Email column appear in the preferences screen with no frontend change, respecting each type's allowsExternalDelivery and default channels. Unit-tested for enqueue-not-inline, message content, deep-link resolution and the host-relative guard; verified end-to-end into the mail catcher. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Localize the notification email chrome Add the email channel's greeting, CTA and footer for de/es/fr/it/no/sv, sitting inline with the other keys, and drop the section comment across every catalog (incl. en) so the keys read like the rest of the file. The title and message still come from the notification; en is the fallback for any missing locale. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Register the transport channels only when a type can use them In a core-only install the sole notification type is the built-in "info" catch-all, which never allows external delivery — so the email channel would be dead weight: an extra column on the preferences screen and an instantiated mailer no notification could ever reach. The dispatch compiler pass now evaluates the registered descriptors and, when none allow external delivery, drops the tagged transport channels entirely (the in-app "popup" substrate is always available and is not a tagged channel). Installing a bundle that contributes an externally-deliverable type — Collab's mention/task/discussion types — brings the channels back automatically, so nothing changes for a real Studio install. A descriptor wired with service references or that fails to construct is assumed external-capable, so a channel a bundle actually wants is never stripped on a false negative. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Use #[AutowireIterator] instead of #[TaggedIterator] Symfony 8 removes the #[TaggedIterator] attribute in favour of #[AutowireIterator], which has an identical constructor. A drop-in rename in the two notification registries; behaviour is unchanged on Symfony 7. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Simplify the notification dispatch pass Extract the definition-skip check and the channel apply/remove loop out of process(), bringing its cognitive complexity back under the threshold. No behaviour change — the gating and tagging are identical. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Define the email routing in config/prepend, matching the bundle convention Every other Messenger message in the bundle declares its transport in a YAML file (execution_engine.yaml, config/prepend/*.yaml); only the notification email routing was inline PHP in the Extension. Move it to config/prepend/notification.yaml, loaded like the other prepend configs. Transport is unchanged (pimcore_core — a fire-and-forget delivery, not a job, so not pimcore_generic_execution_engine). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Make the notification email a complete HTML document The template was a bare <div>. Wrap it in <!DOCTYPE html> + <html lang> + <head><meta charset> + <body>, matching Pimcore's own workflow notification email (Pimcore\Mail does not wrap fragments). Content is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Document how to extend notifications Add doc/03_Extending page: contributing a notification type descriptor and a delivery channel, with the dispatch flow, the 20-char type-id cap, the config envelope and the frontend-renderer pointer. Linked from the chapter index. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Add a <title> to the notification email The <head> needs a title; use the notification's title. Fixes the SonarCloud "Add a <title> tag to this page" reliability bug on the email template. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Validate notification type ids at container build, and stop discarding the runtime failure The docs, the PR description and the exception text all promised that an over-long or duplicated type id would fail at container build. It did not: both checks lived in NotificationTypeRegistry's constructor, so a bad id compiled, deployed and passed CI, and only surfaced as a 500 the first time anyone opened the preferences screen. The failure was also lopsided. NotificationSavedSubscriber wraps the same resolution in catch (Exception) and falls back to showing the toast, so the bell kept working while the preferences screen was dead — and nothing was logged. Move both checks into NotificationDispatchPass, which already materialises descriptors to answer allowsExternalDelivery(). The check is best effort by construction: a descriptor wired with service arguments cannot be read at compile time, so the registry keeps both checks and stays authoritative. The docs now say that rather than overclaiming, and note the consequence contributors need to know — a descriptor's constructor runs during container compilation and must be side-effect free. The swallowed exception in the subscriber is now logged, so a misconfigured descriptor is no longer visible only as one screen failing in isolation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Drop unavailable notification channels on save instead of rejecting the whole request updateSubscriptions() validated every requested channel against the available set and threw, which meant an administrator disabling a channel while the preferences screen was open cost the user every other row in their bulk save — for something they could not influence. It was also inconsistent with itself: a channel the type structurally cannot use was already dropped silently a few lines further down, and resolveChannels' own docblock argued for dropping while the loop above it rejected. The two mechanisms fought each other. Both cases now drop. The endpoint returns the stored state so a dropped channel is visible to the client rather than silent, and it is logged for anyone debugging one. An unknown type id is still rejected, because that is not a race an administrator could have caused and returning state cannot repair it — but as InvalidArgumentException (400) rather than the registry's NotFoundException (404 ELEMENT_NOT_FOUND). It is a bad field in a request body, not a missing resource, and 404 was undocumented in the endpoint's responses. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Ship the notification labels this bundle emits, in all seven locales The backend decides these key strings — GeneralNotificationDescriptor hardcodes the four type keys, and SubscriptionService composes the channel keys from CHANNEL_TRANSLATION_PREFIX and the channel id — but the values lived in studio-ui-bundle, in English only. That put ownership in the wrong place: renaming a channel id here would break a label there with no test failing in either repository, and a backend release would render raw keys until the frontend caught up. Verified that backend-owned keys do reach the UI before moving them: 22 keys in this file are already consumed by the frontend and defined nowhere else (the studio_ee_job_* family), served through the studio domain catalogue by getAllTranslationsByLocale. studio-ui-bundle#3913 must drop these six keys when it lands; the ~20 notifications.settings.* keys stay there, as the frontend composes those itself. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Narrow the materialised descriptor instead of asserting its type SonarCloud php:S1488 on the temporary $descriptor introduced by the previous commit. The variable only existed to carry a /** @var */ annotation, because newInstanceArgs() returns `object` while the method returns ?NotificationTypeDescriptorInterface. Replace the annotation with a real instanceof narrowing. The check is a formality — the caller only reaches materialise() for a class that already passed is_a() — but it narrows the type honestly rather than asserting something PHPStan cannot verify, and it removes the immediate-return Sonar flagged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Trim the comments added by the previous commits Several of them restated the commit-message rationale in the source, or narrated what the code used to do. Kept the why, dropped the archaeology. * Harden the notification dispatcher and the channel gate (#2001) * Gate transport channels by clearing the tag, not removing the definition The gate removed the channel's service definition when no registered type allows external delivery. Aliases and references are not rewritten by removeDefinition(), so the first bundle to alias ChannelInterface to its own channel — exactly what the extending doc shows — got a ServiceNotFoundException at compile time as soon as the gate closed. Untagging achieves the same thing without touching the graph: the registry collects by tag, and an untagged private service nothing references is dropped by Symfony's own unused- definition pass, so no dead mailer is instantiated either way. The new test aliases the interface to a gated channel and compiles the container; it fails with removeDefinition() and passes with clearTag(). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Isolate per-recipient failures in the dispatcher, and make its decisions testable Two problems with one cause: NotificationDispatcher::write() called Notification::save(), which goes through a Dao, so the dispatcher could not be unit tested at all — and the write was the only step in the fan-out that was not isolated. The untestability was not theoretical. testBrokenChannelDoesNotPreventOtherChannelsFrom- Delivering never called dispatch(); it built a TestChannel with throwOnSend and then asserted on ChannelRegistry. TestChannel::$sent was written by the fixture and asserted nowhere in the suite. The resilience guarantee ChannelInterface::send() documents in capitals was unverified, as were the permission skip, the unsubscribed skip and the unknown-recipient skip. Extract NotificationWriterInterface so the dispatcher holds only routing decisions, then wrap the per-recipient body so a failed write is logged and the fan-out continues. Previously a failure part-way through delivered to the recipients before it, silently skipped everyone after it, and surfaced as an exception the producer could do nothing with — while deliver() immediately below already logged and continued. The interface promised the latter behaviour. Eight dispatcher tests now call dispatch() and assert on what the writer and the channels actually received. Both fixes are mutation-checked: removing the guard fails the fan-out test, and the pre-existing behaviour fails the isolation test. The EffectiveSubscription and DispatchableNotification cases move to their own files, so NotificationDispatcherTest is about the dispatcher. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Trim comments --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> * Address review findings on the notification framework NotificationDispatchPass classified every definition in the container with class_exists() plus a fresh ReflectionClass. That is ~6k classes on a real install, and it read the definition's class name raw — so a descriptor or channel registered as class: '%some.parameter%' was silently never tagged, and a class with a missing parent would have raised an uncatchable fatal mid-build. ContainerBuilder::getReflectionClass() resolves the parameter, contains the fatal, and reuses the reflection the rest of the compilation already did. Unsubscribing no longer wipes the stored channels. The switches say nothing while a type is muted, and the resolver ignores them for an unsubscribed type anyway, so overwriting the set only meant that turning a type off and on again left the user subscribed to something that delivered nowhere — not even the pop-up, because a stored empty set reads as a deliberate "none". Storing null where nothing was ever chosen keeps the descriptor defaults reachable too. The producer deep link accepted "//host", which is protocol-relative once the host prefix is empty — and resolveHostUrl() legitimately returns an empty string in a worker with no configured domain, the one case the host-relative guarantee was written for. Also: ChannelInterface still said the bundle ships no channel implementation, which EmailChannel has since contradicted; Installer's new method landed between an @throws docblock and createMcpAccessTokenTable(), leaving one method with two docblocks and the other with none; the extending doc imported UserInterface from the wrong namespace; and UpdateSubscriptionItem accepted non-string channel ids that reached a string-typed closure as a 500. Both behaviour changes are mutation-checked: the three new tests fail against the previous code. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Put the catch-all's labels under the same prefix as the rest The four keys the general descriptor emits were the only notification keys under a singular notification.* prefix; everything else in the domain — the channel and email chrome added here, and the notifications.* family the frontend has shipped for as long as the bell has existed — is plural. Renamed now because these are a one-way door: once released they are in POEditor and in whatever a customer has overridden, and the descriptor's key is what the API hands the frontend to render. notification.type.general.* -> notifications.type.general.*, in the descriptor and all seven catalogues. studio-ui-bundle#3913 still has to drop its copies of these keys; it reads the key from the API response, so nothing there changes beyond the removal already planned. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Document that a new notification group needs a heading label getGroup() reads as self-contained, but the preferences screen composes the heading as notifications.settings.group.<group> rather than taking it from the API the way the row label and description are taken. studio-ui-bundle ships only the "general" key, so a contributed group renders its raw key as the heading. Headings are hidden while there is one group, so the first bundle to contribute a type is exactly the one that surfaces this — including the acme_crm example on this page. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Fix what running the framework against a real install turned up Four things, found by wiring #1959 into the demo app with Collab contributing four externally-deliverable types and driving the preferences screen through Playwright. resolveChannels() no longer keeps the stored channels when a type is switched off. My earlier change did, on the reasoning that re-subscribing should restore them — but the preferences screen clears its own channel set on mute ("Mirrors the server, which clears channels when a type is switched off"), so the client sends an empty set on re-enable and the stored one is overwritten anyway. The change bought nothing and left the two repos describing opposite behaviour. What is worth keeping is narrower and now applies to both branches: a channel id this installation does not offer was never on screen, so neither a save nor a mute is entitled to clear it. The update endpoint documented 400 for its two rejections. Both are InvalidArgumentException, which this bundle maps to 422, so the generated client typed the error wrong. Documented as 422; no behaviour change. The email greeting is "Hi %name%," fed from getFullName(), and a Pimcore user need not have a first or last name — a seeded user produced "Hi ,". Falls back to the username. An email dispatched with no request and no pimcore.general.domain gets a host-relative link, which in a mail client is a dead button. Nothing better can be emitted, but the previous comment called it "a host-relative link rather than a broken absolute one" as though that were fine. It is logged now. Verified against the running app: 22 Playwright specs (11 API + 3 UI, plus setup), the email observed in the mail catcher, and the channel gate confirmed to close in a core-only install and reopen when Collab's descriptors return. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Say when a channel cannot reach the account behind the switch The email switch stores fine and then delivers nothing when the user has no email address on their account, which is indistinguishable from a broken channel — it cost a real debugging session to work out that was all it was. ChannelInterface gains unavailableReasonFor(): a translation key when the channel cannot reach that user, null when it can. It is the channel's own question to answer, so a chat channel can say "no linked account" without the framework knowing what an account is. EmailChannel answers it for a missing address, and the skip in send() is logged rather than silent. The reason travels on AvailableChannel so the preferences screen can explain the column instead of hiding it: the preference is real and starts working the moment an address exists, so hiding the switch would be the wrong fix. Adding the method now costs nothing — the framework is unreleased and EmailChannel is its only implementer. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Gate transport channels at runtime instead of compile time The compiler pass materialised descriptors during container compilation to decide whether any type allows external delivery, and untagged the channels when none did — plus duplicated the type-id validation the registry already performs authoritatively. That was ~120 lines of the trickiest code in the framework (constructors running mid-compilation, best-effort semantics with an assumed-external fallback) for a decision a 5-line runtime check makes with full accuracy. The pass now only tags implementers. The registry keeps validation (its tests already covered duplicate, overlong and at-limit ids independently); SubscriptionService narrows the offered channel columns via the new NotificationTypeRegistry::hasExternallyDeliverableType(). Delivery never depended on the compile-time gate: with no externally-deliverable type the resolver already narrows every subscription to the pop-up. Trade-off: a duplicate or overlong type id now fails on first use of the registry rather than at container build. Also drops the never-called ChannelRegistry::getEnabledChannels(). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Trim comments to the load-bearing why Interface and class docblocks carried multi-paragraph essays; several said the same thing in three places (the null-vs-empty channels rule lived in the entity, the migration and the installer). Comments now state the one non-obvious fact and point elsewhere for the rest. No code changes beyond removed comment lines. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Docs: validation happens in the registry, not at container build Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Docs: neutral example naming instead of ACME Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Docs: task-first restructure of the notifications page Lead with what to do (send / add a type / add a channel) instead of how the dispatcher works internally. The 8-row method table is gone — the example plus two sentences carry it. Mercure/pop-up internals dropped except where they change what an extender must do (the payload privacy warning). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Docs: explain why the dispatcher exists next to NotificationService Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Docs: say plainly what a type and a channel are (row and column) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Docs: PIM-native example (product approved) instead of a CRM deal Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Docs: asset-upload example, picked over the workflow-flavoured one Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Types are data: NotificationType value object replaces the descriptor hierarchy A notification type had no behavior — the descriptor interface was eight getters, and the abstract base class existed only to supply default values and to insure the interface against evolution. Both jobs belong to a final value object with constructor defaults: named arguments read like the configuration they are, the shape is sealed, and a later addition is a new constructor default instead of a BC break. Bundles now register a NotificationTypeProviderInterface returning their NotificationType instances (one provider per bundle, typically). The general catch-all is built by the registry itself from GeneralNotificationType and may not be claimed by a provider; its solo labels move to constants, which also removes the instanceof special-casing in SubscriptionService. Channels deliberately stay an interface: send() and unavailableReasonFor() are real behavior with real dependencies. Registry surface renamed to match (getTypes/getType/hasType/ hasOnlyGeneralType); tag renamed to pimcore.studio_backend.notification_type_provider. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Reuse the canonical element→type mapping in EmailChannel EmailChannel::studioElementType() had its own Asset/Document/DataObject → ElementTypes match — a second copy of ElementProviderTrait::getElementType(). Delegate to the trait instead, so the mapping lives in one place and the email deep link can't silently diverge if it ever changes. The trait throws on an unsupported type where the deep link wants "no segment", so the one behavioural difference is preserved with a narrow catch. Drops four now-unused imports. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Resolve the email host via ToolResolver::getHostUrl() EmailChannel::resolveHostUrl() hand-rolled request-host-else-domain from the RequestStack + getHostname()/getRequestScheme(). That reimplements — less completely (it missed the localhost and non-standard-port handling) — the Tool::getHostUrl() that core's own workflow-notification mail uses. getHostUrl() is already exposed on ToolResolverInterface (via the contract it extends), so delegate to it: same helper, testable, and it internally resolves the current request. The RequestStack dependency is now unused and dropped; the studio-specific "cannot make links absolute" warning stays. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Publish notifications on the recipient's Mercure topic, not the shared one NotificationSavedSubscriber published to Topics::STUDIO — the topic every Studio client subscribes to (StudioTopicProvider) — so a notification's title, message, payload and the recipient's unread count rode the wire to all connected users, with only the frontend's client-side recipient check keeping them out of view. That check cannot prevent reading the data off the socket. Publish to UserTopicService::getUserTopic($recipientId) instead. The recipient already subscribes to their own topic (UserTopicProvider grants only the current user's), so delivery is unchanged while no other client receives it. Guards a null recipient (nothing to deliver to a user topic). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Address review nits: url_path, channel dedup, @internal, test comment - EmailChannel: resolve the Studio base path from pimcore_studio_ui.url_path instead of hard-coding /pimcore-studio, so a customised url_path yields correct email links. Wired in the extension with a fallback default since studio-ui is not a hard dependency. - SubscriptionResolver: array_unique the effective channels so a type declaring duplicate default channels can't make the dispatcher send the same email twice. - NotificationType: drop @internal — it is the object contributing bundles construct via NotificationTypeProviderInterface, i.e. a public extension point. - SubscriptionServiceTest: the rejected-unknown-type comment said 400; the mapped status is 422. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Tighten the review-fix comments Comment-only: trim the Mercure-topic, EmailChannel $studioPath, resolver dedup and extension comments to the load-bearing line. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A replace patch with data: null ("clear the field") threw a TypeError
because handlePatch declared a non-nullable array return while
returning the payload data verbatim. The caller already supports null:
PatchService passes the adapter result straight into setValue(), where
null is the normal way to clear a field.
Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes multiselect PATCH operations so replace can clear a field with null.
Changes:
- Widens the private patch handler return type to
?array(MultiSelectAdapter.php:62). - Correctly addresses the root cause at the adapter boundary; its sole caller already supports nullable results (
MultiSelectAdapter.php:44,54). - No backward-compatibility or documentation impact, but a focused regression test is missing.
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-Authored-By: Claude <noreply@anthropic.com>
|
Contributor
Author
|
Superseded by #2019 — same change rebased onto 2026.2 so it lands on the lower release line and forward-merges up. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Because clearing a multiselect through a merge is a legal operation the core adapter's type signature forbade.
What the merge sent: object 333's
coloris empty; you chose to pull that side over, so the merger savedcolor: { action: 'replace', data: null }— "replace the target's value with nothing", i.e. clear the field. That's the correct payload for this merge, not a frontend bug.Where it exploded:
MultiSelectAdapter::handlePatchhandlesreplaceby returning the payload'sdataverbatim — but its return type was declaredarray, sodata: nullthrows aTypeErrorand the whole PATCH 500s (MultiSelectAdapter.php:66).Why widening to
?arrayis the right fix, not a workaround: the caller already supports null —PatchService::patchEditableDatapasses the adapter's return straight into$element->setValue($key, $value)with no null check, andsetValue(key, null)is Pimcore's normal "clear this field". The add/remove paths in the same adapter also cope with null existing values. Only thereplacepath's signature was narrower than its own contract. One character (?) restores it.Verified
MultiSelectAdapterTest, following theNumericRangeAdapterTestpattern): replace withdata: nullreturns null, replace with values returns them, non-array input returns null. 3/3 green locally viavendor/bin/codecept run Unit.🤖 Generated with Claude Code