-
-
Notifications
You must be signed in to change notification settings - Fork 366
Allow underscore and tilde in URI hostnames as per RFC 3986 #853
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
DannyvdSluijs
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR, nice work. I've left one small suggestion on the test to also include a test case for the tilde character.
Could you also add an entry in the CHANGELOG.md ? This way I can merge the PR without adjustments with saves me some time.
Once done it should be easy to create a new release and the issue in your project(s) should be resolved.
Co-authored-by: Danny van der Sluijs <danny.vandersluijs@icloud.com>
|
Version 6.6.2 was released, see https://github.com/jsonrainbow/json-schema/releases/tag/6.6.2 |
|
Thank you for the super fast response! |
|
WOW! Incredible! |
|
Thank you both. @longwave you’ve played a great part in this as well. Creating a clear and minimal PR, resolving the the feedback points very quickly have helped a lot. |
|
@DannyvdSluijs Expect more PRs coming from us, then! 😄 This is really encouraging downstreams from contributing. 😊 I posted my analysis based on the RFC (which @longwave quoted here) on November 27, 14:59. @longwave created a PR here <24 hours later. And within hours (25.5 hours after my comment), you had already merged this and created a release! 🤯 https://www.drupal.org/project/drupal/issues/3352063 includes a work-around for #427 — see Would you be interested in a PR that introduces recursive |
|
@wimleers if the PR genuinely is a fix for an issue this library has with respect to the draft standards I'm very happy with PR's. Specifically with regards to the recursive ref resolving it would think this is already supported. Although I'm not 100% sure. But in the SchemaStorage::expandRefs() i can see recursive calls to itself. Perhaps a small test from your side could see if this is already solved? |
The implementation of RFC 3986 in the URI validator is more strict than the specification allows. While the RFC says
the host component grammar actually allows more characters than are allowed in DNS names.
This is a problem for the Drupal project which uses this package in conjunction with custom stream wrappers with module names in the host component. Module names conform to PHP function name standards, not DNS standards - that is, they allow underscores but not dashes.
The relevant RFC grammar is
This PR widens the
hostvalidation to allow all characters in theunreservedset. Whilepct-encodedandsub-delimsare also technically allowed, this is the smallest change that will help us out.