Stop inheriting the query and fragment of the base URI by Amoifr · Pull Request #951 · jsonrainbow/json-schema

Amoifr · 2026-09-09T12:13:25Z

Fixes #948.

resolve() reused the parsed base components wholesale, so the base query and fragment survived into the resolved URI.

While reproducing it I found a second half you did not mention: the query of the reference was dropped entirely, not just the base one kept.

$resolver->resolve('bar.json?new', 'http://example.org/foo/x.json?old#frag');
// before: http://example.org/foo/bar.json?old#frag
// after:  http://example.org/foo/bar.json?new

So the rule now follows RFC 3986 section 5.3: a reference carrying a path replaces the query of the base whether or not it has one of its own, only a reference without a path keeps it, and the fragment always comes from the reference.

One detail worth flagging, because it is the trap here. isset($components['query']) is not enough: parse('#/definitions/x') reports an empty query, so testing the key alone would drop the base query for every bare fragment, which is the most common $ref shape in this library. The condition therefore tests for a non-empty value, which is consistent with generate() already omitting an empty query.

Testing

Six cases in a data provider. Five of them fail on main, the sixth is the bare fragment against a base with a query, which already worked and is there to keep it working.

Full suite green (3187 tests), same warning and skips as main, and PHPStan reports no errors.

Copilot

🟡 Changes recommended

Empty references and explicit empty queries still incorrectly inherit base URI components.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Updates URI resolution to follow RFC 3986 query and fragment inheritance rules.

Changes:

Clears inherited queries for path-bearing references.
Ensures fragments come from the reference.
Adds query and fragment resolution tests.

File summaries

File	Description
`src/JsonSchema/Uri/UriResolver.php`	Revises query and fragment resolution.
`tests/Uri/UriResolverTest.php`	Adds regression cases for URI inheritance.

Review details

Files reviewed: 2/2 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.

Amoifr · 2026-09-10T08:27:09Z

+        // RFC 3986 section 5.3: a reference carrying a path replaces the query of the base,
+        // whether or not it has one of its own. Only a reference without a path, such as a
+        // bare fragment, keeps it.
+        if ('' !== $path) {


Correct, and I measured it before changing anything:

resolve('?', 'http://e.org/a/x.json?old') = http://e.org/a/x.json?old

RFC 3986 section 5.2.2 treats a query that is supplied but empty as defined, so it replaces the base one rather than letting it through.

One thing your suggestion did not say but which decides the shape of the fix: parse() cannot tell an empty query from an absent one, because both come out as ''.

parse('#f') => path='' query='' parse('?') => path='' query=''

So the components are no help here and the delimiter has to be looked for in the reference itself, which is why e0f078c does

$hasQuery = false !== strpos(explode('#', $uri, 2)[0], '?');

rather than testing $components['query']. ? and ?#f are both in the provider now.

Amoifr · 2026-09-10T08:27:11Z

+            $baseComponents['query'] = $components['query'];
+        }
+
+        // the fragment always comes from the reference and is never inherited


Correct as well:

resolve('', 'http://e.org/a/x.json?old#frag') = http://e.org/a/x.json?old#frag

RFC 3986 assigns T.fragment = R.fragment with no exception, so an empty reference takes the absent fragment of the reference and keeps only the path and query of the base. My own comment two lines below claimed that invariant while the early return quietly bypassed it.

e0f078c restricts the early return to the case where there is no base to resolve against ('' === $uri && (null === $baseUri || '' === $baseUri)), so everything else goes through component resolution. testResolveEmpty keeps its original case and a second one covers the fragment-bearing base.

Both changes are pinned by tests that fail without them. Full suite green: 3190 tests, and the single remaining warning is on the base branch too.

DannyvdSluijs

Looks all nice, I left one question. Could you also handle the conflict can rebase it onto main?

DannyvdSluijs · 2026-09-09T19:20:13Z

+    /**
+     * @dataProvider queryAndFragmentInheritanceCases
+     */
+    #[DataProvider('queryAndFragmentInheritanceCases')]


Nitpick: Can you explain why we would need both the docblock and attribute version at the same time?

Good catch, there was no reason: the suite runs on PHPUnit 8.5, which only reads the docblock, so the attribute was a habit carried over from other projects. Dropped it in 0f3dbe3, the eight provider cases still run.

RFC 3986 section 5.2.2 treats a query that is supplied but empty as defined, so it replaces the query of the base rather than letting it through, and it takes the fragment from the reference without exception, so an empty reference must not keep the fragment of its base. parse() cannot tell an empty query from an absent one, both being '', so the delimiter is looked for in the reference itself. The early return for an empty reference now applies only when there is no base to resolve against.

@dataProvider

The suite runs on PHPUnit 8.5, which only reads the @dataProvider docblock.

The strict comparison made a null $uri reach explode() and throw a TypeError, where it used to be treated as an empty reference.

Amoifr · 2026-09-23T07:43:10Z

@DannyvdSluijs thanks for the review! Rebased onto main: the two conflicts were tests added side by side by #947 and this PR, both kept.

Re-checking after the rebase turned up something of my own: switching $uri == '' to === made resolve(null, …) throw a TypeError from explode(), where main treats a null reference as an empty one. The interface documents a string and every caller in the library checks is_string() first, but it is a public class, so f417988 casts it back and pins it with a test. Happy to drop that commit if you'd rather keep the documented contract strict. 🙂

DannyvdSluijs requested a balanced review from Copilot September 9, 2026 19:42

Copilot started reviewing on behalf of DannyvdSluijs September 9, 2026 19:43 View session

Copilot AI reviewed Sep 9, 2026

View reviewed changes

DannyvdSluijs self-assigned this Sep 15, 2026

DannyvdSluijs added Needs Review Bug labels Sep 15, 2026

DannyvdSluijs reviewed Sep 22, 2026

View reviewed changes

Amoifr added 4 commits September 23, 2026 09:11

Stop inheriting the query and fragment of the base URI

2d01d8e

Drop the unused PHPUnit data provider attribute

0f3dbe3

The suite runs on PHPUnit 8.5, which only reads the @dataProvider docblock.

Accept a null reference in UriResolver::resolve() again

f417988

The strict comparison made a null $uri reach explode() and throw a TypeError, where it used to be treated as an empty reference.

Amoifr force-pushed the fix-948-uri-resolver-query-fragment branch from e0f078c to f417988 Compare September 23, 2026 07:42

Navigation Menu

Uh oh!

Stop inheriting the query and fragment of the base URI - #951

Stop inheriting the query and fragment of the base URI#951
Amoifr wants to merge 4 commits into
jsonrainbow:mainfrom
Amoifr:fix-948-uri-resolver-query-fragment

Amoifr commented Sep 9, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Amoifr Sep 10, 2026

Uh oh!

Amoifr Sep 10, 2026

Uh oh!

DannyvdSluijs left a comment

Uh oh!

DannyvdSluijs Sep 9, 2026

Uh oh!

Amoifr Sep 23, 2026

Uh oh!

Amoifr commented Sep 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

Amoifr commented Sep 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Testing

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Amoifr Sep 10, 2026

Choose a reason for hiding this comment

Uh oh!

Amoifr Sep 10, 2026

Choose a reason for hiding this comment

Uh oh!

DannyvdSluijs left a comment

Choose a reason for hiding this comment

Uh oh!

DannyvdSluijs Sep 9, 2026

Choose a reason for hiding this comment

Uh oh!

Amoifr Sep 23, 2026

Choose a reason for hiding this comment

Uh oh!

Amoifr commented Sep 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Amoifr commented Sep 9, 2026 •

edited

Loading