fix(events): resolve the schema slug listeners compare against (gated, default off) - #82
Merged
Merged
Conversation
ProductionVersionGuardListener and AutomationCleanupListener each carried a
private extractSchemaSlug() that probed for ObjectEntity::getSchemaSlug() — a
method that does not exist — and then fell back to `@self.schema`, which is the
schema's numeric id. Both then compared that id with `!==` against a slug
literal ('application', 'automation'), so the comparison was always true and
neither handler body has ever executed. No exception, no log line.
Replaces both helpers with one shared ObjectSchemaSlugResolver that resolves
the id to a slug via SchemaMapper::find() (request-cached, and memoised here
including misses so a hot write path does not become an N+1).
The resolver matches the REGISTER as well as the schema. A schema slug is not
unique instance-wide: this instance carries two distinct schemas with the slug
`automation` (ids 71 and 5103), so matching on the schema slug alone would fire
OpenBuild's handlers for another app's objects. Mirrors the register+schema
pair pattern already shipped in petstore and planix.
GATED, DEFAULT OFF — `openbuild.listener_slug_contract`.
Correcting the comparison is not behaviour-neutral. ProductionVersionGuardListener
is a FAIL-CLOSED validation guard: it fails OPEN today, so mismatched production
versions are never blocked, and waking it starts REJECTING writes that currently
succeed. AutomationCleanupListener starts DELETING compiled artifacts on
automation delete. Neither path has ever run, so neither has been exercised
against real data. The flag lets the fix ship and be reviewed without switching
all of that on in one deploy, mirroring openregister#2248's approach to the
sibling ObjectTransitionedEvent defect.
Not addressed here: DocumentGenerationListener and AutomationApprovalTriggerListener
have the same id-vs-slug defect via schemaOf(), but they feed the id into
automation trigger matching rather than a literal comparison, so they need a
separate change.
Contributor
Quality Report — ConductionNL/openbuild @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| composer | ✅ | ✅ 100/100 | |||
| npm | ✅ | ✅ 659/659 | |||
| PHPUnit | ❌ | ||||
| Newman | ⏭️ | ||||
| Playwright | ⏭️ |
Coverage: 89.4% (17/19 statements)
Quality workflow — 2026-08-01 21:41 UTC
Download the full PDF report from the workflow artifacts.
Both listeners gained two constructor params (the slug resolver and the opt-in contract) but neither test was updated — 7 ArgumentCountError errors, so CI was red on all six PHP/NC combinations. The old tests were also not a control. They fed `'@self' => ['schema' => 'automation']` — a SLUG — where MagicMapper writes a numeric ID, which is the exact reason these listeners never ran in production. Passing the fixture the production shape would not produce is how a dead listener tests green. The fixtures now carry ids and the resolver decides, which is what the shipped code does. Added a default-off test per listener asserting that nothing happens at all — not even the slug lookup. That is the merge-safety assertion: the production version guard is fail-closed and currently fails open, so waking it starts rejecting writes that succeed today. 739 tests, 0 failures (was 737 with 7 errors).
Contributor
Quality Report — ConductionNL/openbuild @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| composer | ✅ | ✅ 100/100 | |||
| npm | ✅ | ✅ 659/659 | |||
| PHPUnit | ✅ | ||||
| Newman | ⏭️ | ||||
| Playwright | ⏭️ |
Coverage: 89.4% (17/19 statements)
Quality workflow — 2026-08-02 07:18 UTC
Download the full PDF report from the workflow artifacts.
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This fixes a comparison that has never once been true. Correcting it wakes two listeners that have never executed, one of which is a fail-closed validation guard. The fix therefore ships behind
openbuild.listener_slug_contract, default off, mirroring how openregister#2248 shipped the siblingObjectTransitionedEventdefect.The bug
ProductionVersionGuardListenerandAutomationCleanupListenereach carried a privateextractSchemaSlug()that:ObjectEntity::getSchemaSlug()— a method that does not exist, then@self.schema, which is the schema's numeric id.Both then compared that id against a slug literal:
ObjectEntity::$schemais declared?stringbutSaveObjectwrites it as(string) $schemaId, so it holds"28", never"application". Every call returned at the guard. No exception, no log line — the listeners simply did nothing, silently, for their entire life.The fix
One shared
ObjectSchemaSlugResolverreplaces both private helpers, resolving the id viaSchemaMapper::find()(already request-cached; memoised here including misses so a hot write path does not become an N+1 — docudesk measured 1,471find()calls per object save from exactly this shape).It matches the register as well as the schema. A schema slug is not unique instance-wide: this instance carries two distinct schemas with the slug
automation(ids 71 and 5103). Matching on schema slug alone would fire OpenBuild's handlers for another app's objects. This mirrors the register+schema pair pattern already shipped in petstore and planix.ProductionVersionGuardListener$event->stopPropagation()+setErrors()).AutomationCleanupListenerNeither has ever executed, so neither has ever been exercised against real data. Enabling is a rollout decision, not a bug fix:
Verification status — please read
php -lclean;phpcsclean on all four touched files (remaining warnings are pre-existing missing@spectags — I have not invented spec paths).automationcollision and the id-vs-slug shape are both verified directly against the database, not inferred.Not addressed here
DocumentGenerationListenerandAutomationApprovalTriggerListenercarry the same id-vs-slug defect viaschemaOf(), but they feed the id into automation trigger matching rather than a literal comparison, so they need a separate change. (AutomationApprovalTriggerListeneralready injectsSchemaMapperand resolves correctly in the other direction at line 429 — the pattern is present in the file, just not applied to the trigger side.)HybridMetadataLockListenerandApprovalOutcomeListenerare not affected: the former documents this exact id problem and deliberately uses payload-shape detection instead.