fix: nested schema output, custom keywords, and named properties - #34
Merged
Merged
Conversation
tymondesigns
marked this pull request as ready for review
September 15, 2026 22:00
Reuse property()/requireProperties() from #33 instead of adding propertyMap()/require(). Nested items omit $schema; numeric-string property keys compare as strings; empty examples are omitted; keyword() covers unmodelled/vendor keywords. Co-authored-by: Sean Tymon <tymondesigns@users.noreply.github.com>
cursor
Bot
force-pushed
the
cursor/json-schema-nested-schema-fixes-dfaf
branch
from
September 15, 2026 22:09
8870c1e to
c8590f0
Compare
Annotate mixed schema arrays in the JS-* Pest cases and apply Pest coding-style rector so format:check stays clean on PHP 8.4. Co-authored-by: Sean Tymon <tymondesigns@users.noreply.github.com>
covers(AbstractSchema::class) made Pest mutation run only KeywordSchemaTest against AbstractSchema, dropping the suite below the 80% mutation floor. Point covers at the trait under test instead. Co-authored-by: Sean Tymon <tymondesigns@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Numeric-only property names still serialize invalidly, and the public interface change introduces an undocumented compatibility break.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Fixes nested schema serialization and expands object/property and custom-keyword APIs.
Changes:
- Removes invalid nested
$schema, redundant titles, and empty examples. - Adds custom keywords and named/required property support.
- Adds documentation and regression tests.
File summaries
| File | Description |
|---|---|
src/Contracts/JsonSchema.php |
Adds the custom keyword contract. |
src/Types/AbstractSchema.php |
Integrates custom keyword serialization. |
src/Types/Concerns/HasItems.php |
Corrects nested item serialization. |
src/Types/Concerns/HasKeywords.php |
Implements arbitrary keyword support. |
src/Types/Concerns/HasMetadata.php |
Omits empty examples. |
src/Types/Concerns/HasProperties.php |
Expands named and required property handling. |
tests/Unit/Types/ArraySchemaTest.php |
Tests nested item output. |
tests/Unit/Types/KeywordSchemaTest.php |
Tests custom keywords. |
tests/Unit/Types/ObjectSchemaTest.php |
Tests property API changes. |
tests/Unit/Types/StringSchemaTest.php |
Tests empty examples. |
tests/Unit/Converters/ClassConverterTest.php |
Updates nested schema expectations. |
tests/Unit/Converters/ClosureConverterTest.php |
Updates nested schema expectations. |
docs/json-schema/introduction.mdx |
Documents new behavior. |
docs/json-schema/schema-types/object.mdx |
Documents object property APIs. |
Review details
- Files reviewed: 14/14 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
PHP stores "0"/"1" keys as ints, so json_encode emitted a properties array. Cast list-shaped maps to objects so the output stays valid JSON Schema. Co-authored-by: Sean Tymon <tymondesigns@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Named variadic property arguments produce incorrect diagnostic indexes, and the feature overview drops supported contains validation.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
src/Types/Concerns/HasProperties.php:67
- Named arguments passed to a variadic parameter retain their string keys, so
properties(first: ..., second: $untitled)castssecondto0and incorrectly reportsProperty 1. Reindex the arguments before iterating so the new diagnostic always uses the actual 1-based position.
docs/json-schema/introduction.mdx:146 - This replaces the existing “Contains Validation” entry even though
contains()remains supported and documented. Keep that feature in the overview and add custom keywords as a separate bullet.
- Files reviewed: 14/14 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Untitled-property errors used the variadic array key as the 1-based index, so named arguments reported Property 1. Reindex before iterating. Keep Contains Validation in the introduction overview alongside Custom Keywords. Co-authored-by: Sean Tymon <tymondesigns@users.noreply.github.com>
tymondesigns
deleted the
cursor/json-schema-nested-schema-fixes-dfaf
branch
September 15, 2026 22:38
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.
Fixes several schema-output bugs and fills gaps in the object/keyword APIs. Existing
properties(),required(),property(),requireProperties(), andrequireAll()behaviour is preserved.Nested
itemswere invalid JSON SchemaArray item schemas were serialised as if they were document roots. That leaked a nested
$schema(not allowed under JSON Schema 2020-12 unless the subschema is a resource root) and a redundanttitle. Nesteditemsnow matchtupleItems,additionalItems, andproperties: no dialect URI, no leftover title.Numeric property names restated themselves as
title, and numeric-only maps encoded as arraysPHP coerces array keys like
"0"and"1"to integers. Title-vs-name de-duplication compared those keys strictly, so a property named"0"kept"title": "0"in the output. Comparison andgetPropertyKeys()now treat names as strings.A map of only numeric names was also a PHP list, so
toJson()emitted"properties":[...]. JSON Schema requires an object; list-shaped maps are now cast before encoding.Empty
exampleslists were publishedexamples([])wrote"examples": []. Empty lists are now omitted; non-emptyexamples()is unchanged.Untitled nested properties had no usable API
properties()still requires each child to have a title (that title is the property name). There was no way to name a property independently oftitle, including anonymous nested objects.This now uses
property()from #33. The key is the property name;titlestays metadata; untitled nested schemas are allowed. Childrequired()is honoured as well as therequired:flag.When
properties()still rejects an untitled child, the error now names the parent, 1-based argument position, and offending class, e.g.Property 2 of "Wrapper" (Cortex\JsonSchema\Types\ObjectSchema) must have a title. Named arguments (first:,second:) keep string keys in PHP, so the index is taken from argument order rather than the key name.No escape hatch for unmodelled keywords
Draft keywords we do not model, OpenAPI
discriminator, andx-*extensions could not be attached without a special schema subclass.keyword($name, $value)is now available on every schema, including nested ones. NestedJsonSchemavalues serialise without$schema.Runtime required lists needed
requireAll()or per-childrequired()There was no counterpart to
requireAll()for a computed list of names (and those names did not have to exist yet). That isrequireProperties()from #33:requireAll()now uses stored property keys, so it works for untitled schemas added viaproperty().Docs
Documented that repeated
properties()/property()calls merge (union); the last schema for a given name wins. Custom keywords are listed as an additional feature;contains()remains in the overview.