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

league\uri as a PSR's UriInterface implementation #296

Closed
wants to merge 3 commits into from

Conversation

alexeyshockov
Copy link
Collaborator

A proposal to use league\uri package instead of the current Uri implementation. My main goal for this is to get rid of PHP's parse_url() issues with internationalized domain names (see #269 and guzzle/guzzle#2286).

It's a BC break, but has not been released yet, so IMO it's good chance for a change like this.

@Nyholm
Copy link
Member

Nyholm commented Oct 24, 2019

Couldnt we just fix the parse_url issues? Or use https://github.com/thephpleague/uri-parser?

@GrahamCampbell
Copy link
Member

Well, is there a point in having two implementations? Seems like s good idea to reuse leagues?

@alexeyshockov
Copy link
Collaborator Author

@Nyholm, league/uri-parser is deprecated, league/uri should be used instead.

As @GrahamCampbell already said, my main idea is to decrease the product scope. UriInterface is already implemented (in league/uri, for example), so why does Guzzle need to maintain it's own implementation? My fix in #297 is a bit hacky and doesn't fix other bug that PHP's parse_uri() contains. And it seems that PHP's parser will stay the same because of backward compatibility.

So if we want to have a better parser for 2.x (hacks around parse_url() don't count), why would we write another one from scratch when league/uri has implemented it already?

@alexeyshockov
Copy link
Collaborator Author

And yes, we can use just a URI parser from league/uri, but why not take the whole URI implementation? Less code to maintain here.

@Tobion
Copy link
Member

Tobion commented Jan 19, 2020

While I know that PHPs parse_url has broken edge cases, I cannot accept the PR like this. The BC break is far too big. If it's just about replacing parse_url with league/uri parsing, I would consider this. This way the end user would not be affected and the change is a pure internal implementation.

@Nyholm
Copy link
Member

Nyholm commented Jan 19, 2020

I’m also 👎

@Nyholm Nyholm closed this Jan 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants