Skip to content

Conversation

@alcohol
Copy link
Contributor

@alcohol alcohol commented Dec 8, 2025

Description

Update tests (and code to follow) for UriValidator file: protocol.

Related Issue

#856

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Code refactoring
  • Other (please describe):

Checklist

  • I have read the CONTRIBUTING guidelines
  • My code follows the code style of this project
  • I have added tests that prove my fix is effective or that my feature works
  • All new and existing tests pass
  • I have updated the documentation accordingly
  • My changes generate no new warnings

Additional Notes

@alcohol alcohol changed the title Main UriValidator file: schema Dec 8, 2025
@alcohol alcohol changed the title UriValidator file: schema UriValidator file: tests/fix(es) Dec 9, 2025
@alcohol alcohol force-pushed the main branch 3 times, most recently from 1cc4b6e to 07afbe1 Compare December 10, 2025 08:14
- expand uri validator tests
- treat file as non-hierarchical
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This pull request attempts to add support for file: URI protocol validation by updating both the test suite and the UriValidator implementation. However, the implementation has a critical architectural flaw: it treats file: URIs as non-hierarchical when they are actually hierarchical URIs according to RFC 8089 and RFC 3986.

Key changes:

  • Added three test cases for different file: URI formats (implicit localhost, explicit host, and without host)
  • Modified the non-hierarchical URI regex pattern to include file: scheme
  • Added a comment acknowledging limitations with regex-only validation

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File Description
tests/Tool/Validator/UriValidatorTest.php Adds three valid file: URI test cases and a commented-out invalid case with a note about regex validation limitations
src/JsonSchema/Tool/Validator/UriValidator.php Adds file: to the non-hierarchical URI pattern, incorrectly classifying it as non-hierarchical

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Collaborator

@DannyvdSluijs DannyvdSluijs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for persisting and make a very dense fix for the issues.
I have left one remark to remove the comment case in the invalid data provider.

Finally could you add an entry to the changelog as well that would complete the PR and I can merge and release quickly after.

Comment on lines +53 to +54
// Cannot be validated properly with regex only
// yield 'Invalid file uri (missing host)' => ['uri' => 'file://path/to/file.txt'];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you remove these lines as I would rather keep commented code out of the repo to keep it clean. Since there is no invalid file uri we can just skip this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants