Skip to content
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

parsing the empty-string should throw an exception instead of returning a valid instance. #151

Open
nyamsprod opened this issue Jan 1, 2025 · 5 comments
Assignees
Labels
bug Something isn't working version 8.0 Version 8

Comments

@nyamsprod
Copy link
Member

Bug Report

(Fill in the relevant information below to help triage your issue.)

Information Description
Package uri-interfaces
Version all
PHP version any
OS Platform all

Summary

According to RFC3986 an empty string or a string containing spaces are an invalid URI. While the
changes are minimal and represents mostly edge cases they still need to be addressed sooner rather than later and represents BC break in the package.

Standalone code, or other way to reproduce the problem

UriString::parse('');
Http::new();
Uri::new();
Uri::new('https://example.com/ path?query#fragment.');

Expected result

All the following URI should throw a SyntaxError or a ValueError exception instead of returning a valid URI instance.

Actual result

returns a parse/valid URI instance.

Backward Incompatible Changes

Fixing this issue will trigger a BC break in the following packages:

  • uri-interfaces;
  • uri;
  • and the uri-components;

👉🏾 All URI related objects expecting an empty string and which currently return a valid instance should no longer do so.
👉🏾 The path component can be nullable currently it is always set which is not what the RFC expects.

@nyamsprod nyamsprod added bug Something isn't working enhancement New feature or request labels Jan 1, 2025
@nyamsprod nyamsprod self-assigned this Jan 1, 2025
@nyamsprod
Copy link
Member Author

nyamsprod commented Jan 1, 2025

The part where URI string containing spaces are valid is fixed via the following commit without BC break. What still need to be fixed is an URI represented by the empty string.

If you are using any version prior to v7.6.0 and to fix this issue you are required to filter the URI before you submit it to the parse method.

$uri = trim((string) $uri);
if ($uri === '' || str_contains($uri, ' ')) { //the 2 checks are performed
    throw new ValueError('The uri cannot be an empty string or contain spaces.');
}

$component = UriString::parse($uri);
$uriObject = Uri::new($uri);

If you are using a v7.6 and before v8 you need to do the following:

UriString::parse($uri); // is fixed, it throws a SyntaxError

$uri = trim((string) $uri);
if ($uri === '') { //only checking the empty string
    throw new ValueError('The uri cannot be an empty string.);
}
$uriObject = Uri::new($uri);

In v8

UriString::parse($uri); // throw
Uri::new($uri);         // is fixed, it throws a SyntaxError and it is a BC break from `v7`

@nyamsprod nyamsprod added the version 8.0 Version 8 label Jan 1, 2025
@nyamsprod
Copy link
Member Author

@kelunik @shadowhand I will try not promise to release v7.6 this week (still in holidays) but the question is should I also try to release v8 ASAP or can this still wait until #137 to be at least discussed (from my last comment the only thing I am a bit not yet settled on is the need of all the current interfaces 🤔.

@nyamsprod nyamsprod removed the enhancement New feature or request label Jan 2, 2025
@shadowhand
Copy link
Member

@nyamsprod

The uri can not be the empty string or contain a space

This should be rephrased as:

The uri cannot be an empty string or contain spaces

@shadowhand
Copy link
Member

@nyamsprod regarding the question of #137, my previous opinions stand. I do think dropping the interfaces makes sense.

@nyamsprod
Copy link
Member Author

nyamsprod commented Jan 2, 2025

@shadowhand thanks for the input I will rephrase it in the changelog 👍🏾.

Meanwhile I was able to completely fix the parser without BC breaking the URI and the Http instances see the new commit to add the BC break whenever v8 is released.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working version 8.0 Version 8
Projects
None yet
Development

No branches or pull requests

2 participants