Draft-aware $ref+sibling handling (Draft 2019-09+) - #185
Merged
Conversation
Introduce PropertyProducerInterface for keywords that resolve/replace a property (which ModifierInterface::modify, returning void, cannot do), register it per-draft via DraftBuilder/Draft, and relocate $ref resolution into a dedicated RefResolver. Draft-07 wraps it in ExclusiveProducer (siblings ignored); Draft 2019-09 overwrites it with a bare resolver (siblings will apply). PropertyFactory::create now dispatches directly to the resolver; registry-based dispatch and sibling orchestration land in a follow-up phase. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Introduces a keyword-keyed producer registry on Draft that routes property-producing keywords (starting with $ref) through PropertyProducerInterface rather than hard-coded branches in create(). - Draft::getProducersForSchema returns keyword-keyed producers present in a given schema node so callers can name conflicting keywords in error messages - PropertyFactory::create dispatches through produceProperty when any producers fire; an ExclusiveProducer suppresses all other producers on the same node, and two simultaneous exclusive producers throw SchemaException to prevent silent winner selection - GeneratorConfiguration::getBuiltDraft centralises draft resolution and build with a class-keyed cache, eliminating the duplicate private resolveBuiltDraft present in both PropertyFactory and FilterProcessor - FilterProcessor now delegates to getBuiltDraft instead of its own copy Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Draft 07: $ref is exclusive — sibling keywords are silently ignored at the base level (JsonSchema constructor no longer wraps them in allOf). Draft 2019-09+: $ref and its sibling keywords apply simultaneously; PropertyFactory processes siblings first so they are root-registered before RefResolver contributes ref properties with allOf semantics for name collisions, preserving type-intersection narrowing and default-conflict detection. Tests split Issue79Test by draft and extend ReferencePropertyTest with dedicated RefWithSiblings schema covering merge correctness, authored JSON pointer locations (no synthetic /allOf/ segment), and Draft 07 sibling-drop behaviour. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
For non-exclusive producers (Draft 2019-09+), a property-level $ref with scalar or array siblings (minLength, type, enum, const, …) now merges the ref-resolved type with the local sibling keywords instead of ignoring them. - Builds an exclusively-owned target property to avoid mutating the shared ref-resolved property (D15 constraint). - Defers the merge via onResolve for recursive-ref safety (D17). - Resolves the effective PHP type as the intersection of the produced type and any explicit sibling 'type' keyword; an empty intersection throws SchemaException at generation time. - Applies type-specific sibling modifiers (minLength, pattern, minimum, …) and 'any' modifiers (default, enum, const) on the target property. - Transfers non-TypeCheck validators from the ref property; decorators are intentionally excluded (type-conversion decorators target the produced type, not the narrowed effective type). - Adds TypeConverter::phpToJsonSchema() reverse mapping. - Object×object merge (ref resolves to an object schema with structural sibling keywords, or two co-occurring object producers) is deferred pending save/restore of SchemaProcessor::currentClassPath. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Property-level scalar/array $ref+sibling merge (path 3 in mergeProducedPropertyWithSiblings): re-runs the ref's type-specific modifiers with the effective (narrowed) type so that range validators use the correct PHP type-check function (is_int instead of is_float when narrowing number→integer). Fixes validator transfer for numeric constraints from $ref definitions. - Nullable narrowing: when a concrete non-null sibling type constrains a nullable ref (string|null + type:string → string), the effective PropertyType is set non-nullable so the PHP type hint reflects the narrowed type. - New RefSiblingsTest: 15 tests covering non-structural sibling keywords (minLength, enum, const) applied in Draft 2019-09+ and silently ignored in Draft 07; scalar type narrowing (number→integer) with ref constraint preservation; nullable narrowing; type contradiction SchemaException; bare $ref unchanged across all drafts; authored JSON pointer assertions (no /allOf/N). - RefSiblingsPropertyLevelTest: pointer assertions for merged nested class (ref properties keep /definitions/... pointers, sibling properties keep authored /properties/... paths). - Issue79Test: pointer assertions confirming no synthetic /allOf/N segment on root-level $ref+siblings in Draft 2019-09+. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… $ref+sibling base-level merge When a property is defined by both a $ref'd object and a sibling 'properties' entry at base level (Draft 2019-09+), three new behaviours are now correct: 1. Type intersection: the effective PHP type is the allOf intersection of the ref and sibling types (e.g. number ∩ integer = integer). After narrowToIntersection strips the TypeCheckValidator from the existing slot, RefResolver::reapplyTypeSpecificRefConstraints re-applies all type-specific modifiers from the ref's JSON with the narrowed effective type substituted — restoring the TypeCheckValidator and adding constraint validators (minimum, maximum, etc.) using the correct type-check function for the narrowed type (is_int after integer narrowing, not is_float). 2. Constraint propagation: value constraints from the ref definition (minimum, maximum, etc.) are now transferred to the merged property with the narrowed type, so e.g. minimum:0 from a number-typed ref remains active after narrowing to integer. 3. Default conflict detection: PropertyMerger::reconcileAllOfDefaults detects and rejects schemas where $ref and sibling declare conflicting default values for the same property, and propagates one-sided defaults (ref → sibling or sibling → ref) when only one side carries a default. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ide-allOf, and JsonSchema attribute
- Recursive $ref + siblings: verifies no crash when the referenced object has a
recursive self-reference; draft-specific behaviour matches root-level expectations
- External-file $ref + siblings: proves sibling merging works identically for refs
that point to an external JSON file rather than an internal definition
- {$ref, siblings} inside an allOf branch: confirms draft-aware dispatch applies at
every nesting depth, not only at the root and direct-property levels
- #[JsonSchema] attribute: verifies the embedded schema JSON contains $ref directly
and carries no synthetic allOf wrapper introduced by the old constructor wrap
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add a new '$ref with sibling keywords' section explaining: - Draft 2019-09+: siblings alongside $ref are applied (merging properties, narrowing types, enforcing constraints from both sides) - Draft 7: siblings are silently ignored (ExclusiveProducer, spec-correct) - Upgrade note: schemas that mix $ref + siblings will behave differently if the draft changes from 7 to 2019-09 Includes worked examples for both base-level (object merge) and property-level (scalar constraint narrowing) sibling usage. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Reformat a heredoc regex pattern into a concatenated string to stay within the 120-character line limit. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add rawSource parameter to JsonSchema constructor (master error-location improvement) - Keep producer-based $ref dispatch in PropertyFactory (feature branch approach) - Reformat long data-provider entries in ReferencePropertyTest per base branch style Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…erties When a property schema contains a bare $ref (no siblings), PropertyFactory::create() routes to produceProperty() which delegates to the producer's produce() method. The $isArrayItem flag was silently dropped at this boundary, so SchemaDefinition::resolveReference() received isArrayItem=false regardless of the call site. This caused TypeCheckModifier to compute allowImplicitNull=true for the underlying definition property (because !property->isRequired() was true), inserting a `&& $value !== null` guard into the TypeCheckValidator's check string. For inline nested-object schemas the flag reaches buildProperty() correctly and isRequired() returns true, so no null guard is added. The fix threads $isArrayItem through the full producer call chain: PropertyProducerInterface::produce() → ExclusiveProducer → RefResolver::produce() → resolveReference() / resolveBaseReference() and includes isArrayItem in the SchemaDefinition cache key. Array-item usages now get their own cached property with the correct TypeCheckValidator (no null guard), while non-array-item usages continue to receive the null guard for optional properties. Also update stale ContainsException message assertions in ArrayContainsTest (property name is now quoted in the message) and fix InvalidTypeException assertions in Issue168Test and ReferencePropertyTest to match the production library's updated wording format. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…-draft-aware # Conflicts: # tests/Issues/Issue/Issue168Test.php
…s for uncovered paths PropertyProxy was maintaining a local $isProxyArrayItem field and tracking setArrayItem/isArrayItem locally to ensure isRequired() reflected array-item context. After SchemaDefinition included isArrayItem in its cache key (each path+context combination gets a distinct underlying property), the local tracking became dead code — delegate to the underlying property directly. Similarly, the RequiredPropertyValidator guard in PropertyFactory's transferProducedValidators() was unreachable: RequiredValidatorFactory attaches to the target property after the onResolve callback fires, so $refProperty never carries one. Add integration tests covering the previously-uncovered paths: - Draft.getProducerForKeyword() (both registered and unknown keyword) - RefResolver: empty-string $ref hits the final unresolved-reference throw - RefResolver.reapplyTypeSpecificRefConstraints: multi-type intersection (count != 1) triggers early return - PropertyFactory.applyScalarSiblingMerge: untyped $ref with scalar sibling - PropertyFactory: colliding property name between $ref object and sibling uses allOf semantics - PropertyFactory: structural sibling with non-object $ref throws SchemaException - EnumPostProcessor.hasEnumFilterAlreadyApplied: two composition branches sharing the same $ref do not apply the EnumFilter twice Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
An empty $ref ("") is a URI-Reference with an empty path/fragment (RFC 3986
§5.2.2), which resolves to the current base URI - the enclosing document or
$id scope, not always the top-level document. SchemaDefinitionDictionary
previously treated it as unresolvable, and both internal and external
reference resolution only ever consulted the top-level document's own $id,
ignoring any nearer enclosing $id a $ref was actually nested under.
- JsonSchema now tracks the nearest enclosing $id while navigating the tree
(JsonSchema::getBaseId()), and SchemaDefinitionDictionary resolves an
empty $ref against it - both for internal fragment lookups and for
relative external-file $refs written inside a nested $id scope.
- RefResolver now rejects an empty/root $ref at a schema's own base level
with a SchemaException instead of recursing forever trying to merge the
schema with itself, which has no fixed point.
- Fixed a test data-provider that treated "" as interchangeable with named
definitions.person references; it isn't - it targets the enclosing scope
itself. Added dedicated tests for the legitimate patterns instead.
- tests/bootstrap.php's shutdown-time cleanup now suppresses rmdir/unlink
warnings: PHPUnit >=13.2.0 turns an unsuppressed warning raised outside a
running test into an uncaught NoTestCaseObjectOnCallStackException,
crashing the whole run instead of just leaving a temp dir behind.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Coverage Report for CI Build 31429310725Coverage increased (+0.05%) to 98.811%Details
Uncovered Changes
Coverage Regressions2 previously-covered lines in 1 file lost coverage.
Coverage Stats
💛 - Coveralls |
…econciliation PropertyMerger::reconcileAllOfDefaults() has three call sites; existing RefSiblingsTest coverage only exercised the typed-intersection path (narrowToIntersection). The other two - a genuinely untyped $ref'd property, and an explicit "type":"null" sibling merged via mergeIntoExistingNull() - were never reached by any test. Verified each new test actually depends on its target call by temporarily removing it and confirming the test fails, then restoring. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
wol-soft
commented
Aug 5, 2026
A probe surfacing an unexpected result from valid input is evidence of a real defect, same as a formal test would be - silently editing the probe until the inconvenient result disappears erases that evidence instead of investigating it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…-draft-aware Pulls in the AutoDetectionDraft fix (#187) for property-level $schema inheritance/override, which this branch's own investigation (see .claude/topics/property-merger-consolidation/) had independently found and was about to fix separately. # Conflicts: # src/Model/SchemaDefinition/JsonSchema.php
RefResolver's base-level merge and PropertyFactory's property-level scalar merge each had their own copy of "narrow the type across the $ref boundary, then rebuild type-specific validators for the narrowed type from the ref's original JSON" - the property-level object-merge collision loop had no such step at all. Route all three through PropertyMerger instead, so there is one implementation instead of two (soon three) drifting copies. Extend PropertyMerger::merge()/narrowToIntersection() with optional $rebuildFrom/$schemaProcessor/$schema, move the reapply logic in from RefResolver verbatim, and wire it through Schema::addProperty() to all three call sites. Two real bugs surfaced while doing this, neither previously covered by any test: - The reapply step only ran when narrowing actually changed the type. When the sibling's type already equalled the intersection (the common case), the ref's own range constraints (minimum, etc.) were silently dropped regardless - fixed to reapply unconditionally, matching the original RefResolver behavior which always re-applied after any collision. - PropertyFactory's scalar merge path never reconciled default values at all, silently keeping one side's default on conflict with no error - unlike the base-level path, which already throws for this. Fixed as a side effect of routing through PropertyMerger, once the sibling's own "default" is applied before the merge instead of after (applying it after was silently overwriting whatever the merge had resolved before it was ever compared against the ref's). New regression tests for both, each verified by reverting its fix and confirming the test fails before restoring. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
wol-soft
commented
Aug 9, 2026
- Derive the object structural-keyword list in PropertyFactory from the draft's own registered validators (Draft::getKeywordsForType()) instead of a hardcoded, partly-aspirational constant, so it stays in sync as drafts add real keyword support. - Keep GeneratorConfiguration's public draft API unchanged (getDraft/ setDraft/getBuiltDraft) while moving the built-draft cache into a private DraftResolver, since PostProcessor's public extension contract only ever provides GeneratorConfiguration, not SchemaProcessor. - Document (with a regression test) why resolveReference()'s cache key must include isArrayItem unconditionally: gating it behind implicit- null would let an array-item and a plain optional-property usage of the same $ref collide, corrupting the optional property's nullability. - Merge the two type-narrowing tests in RefSiblingsPropertyLevelTest into one, with full exception-message assertions and a getter type-hint check. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
AbstractPHPModelGeneratorTestCase already provides getReturnTypeNames() for asserting nullable/union type hints; use it instead of manually building a ReflectionClass/ReflectionType assertion, consistent with how the rest of the suite asserts getter type hints. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
wol-soft
added a commit
that referenced
this pull request
Aug 12, 2026
Four crashes and false rejections, each verified against master to separate
regressions from pre-existing behaviour.
An allOf branch whose declared type is a LIST containing "object" was rejected
as conflicting with an object-asserting sibling. assertNoObjectScalarTypeConflict()
inferred "scalar branch" from the absence of a nested schema, but a multi-type
branch has none either: createMultiTypeProperty() puts it on the object
sub-property it builds, which is unreachable from the branch. That rejected
`allOf: [<object>, {"type": ["object", "null"]}]` - the ordinary "referenced
type, but nullable" shape - as unsatisfiable. The check now reads the branch's
declared type. Deliberately not routed through ObjectShapeResolver: that answers
whether a branch ASSERTS object-ness, which a multi-type branch does not, while
the question here is the weaker "can an object satisfy this branch at all".
Two cross-file reference shapes broke. checkObjectRepresentability() ran before
generateModel() registered the schema, and its $ref peek re-enters
processTopLevelSchema() for cross-file targets, so parseExternalFile()'s dedup
short-circuit could not fire: two files referencing each other recursed until the
stack was exhausted, and a target referencing back built a second render job
("File X.php already exists"). Both registrations now happen before the check.
ObjectShapeResolver's own cycle guard cannot cover this - it is local to one
classify() call and cannot see re-entrancy through the SchemaProcessor.
A composition keyword given as a JSON object rather than an array made every
branch-numbering site compute $index + 1 on a string key, and a branch that is
not a schema was indexed into as an array. Both now raise a SchemaException
naming the offending keyword and branch. `not` and if/then/else crashed the same
way on a scalar branch - pre-existing rather than a regression, but hardened
alongside, since rejecting a malformed allOf branch cleanly while `not` still
fatals is a half-fix.
The representability message withheld its fix suggestion from NotObject schemas.
The flag is correctly not offered there (it only ever widens acceptance to
ObjectDescribing), but declaring the type does resolve them, so it is now named -
conditionally, because unlike a describing schema a NotObject one really does
accept non-object values, so declaring the type changes the accepted set rather
than stating what the schema already means.
Ten docblocks still named PropertyFactory::processBaseReference()/
processReference() and PropertiesValidatorFactory::addDependencyValidator() after
the #185 rework moved them; a filter docblock still described the Blocking
verdict that became Undecidable; PropertyFactory carried an unused Exception
import.
Documentation: mergedProperty.rst documented a _Merged_ class for an allOf
example, and allOf.rst and oneOf.rst both promised one. Established by generating
each shape and reading the output: anyOf is the only keyword that produces a
merged property - OneOfValidatorFactory never calls createMergedProperty() at
all, and an allOf of object branches is routed through the object path to a
regular nested class. The allOf-inline and oneOf claims were already stale before
this branch; the $ref-based allOf form is what it changed. impliedObjects.rst
also claimed every object-describing schema warns, which is untrue of a
dependencies value - that site force-asserts silently.
New coverage for each fix, plus the bare object-describing array item this branch
introduced and the dependencies site's asymmetry, which was recorded nowhere
before. Two further shapes found while probing are filed as issues rather than
pinned as tests (#189, and a comment on #181): a test asserting a TypeError
documents a bug as if it were a contract.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
2 participants
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.
Summary
Implements draft-aware `$ref`+sibling keyword handling for JSON Schema Draft 2019-09 and later, where `$ref` no longer discards sibling keywords but merges them with the referenced schema.
Test plan
🤖 Generated with Claude Code