Skip to content

POC: Add ext/url #11315

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

Closed
wants to merge 3 commits into from
Closed

POC: Add ext/url #11315

wants to merge 3 commits into from

Conversation

kocsismate
Copy link
Member

This PR is purely a POC which adds support for a new extension called url building upon https://github.com/ada-url/ada. The 2 tests serve as a very simple benchmark.

@@ -3628,7 +3628,6 @@ function uniqid(string $prefix = "", bool $more_entropy = false): string {}

/**
* @return int|string|array<string, int|string>|null|false
* @compile-time-eval
Copy link
Member Author

Choose a reason for hiding this comment

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

I only removed this just to be able to compare its real performance

@kocsismate kocsismate force-pushed the ext-url branch 2 times, most recently from e67f6bb to 9365001 Compare May 26, 2023 17:44
@kocsismate kocsismate closed this May 28, 2023
@anonrig
Copy link

anonrig commented Jun 12, 2023

Hello! I'm one of the authors of Ada URL. How was your local benchmarks? I'd like to help and understand why this PR was closed.

@NattyNarwhal
Copy link
Member

I wonder if curl's urlapi might be useful, since it'd be consistent with ext/curl.

@kocsismate
Copy link
Member Author

I wonder if curl's urlapi might be useful, since it'd be consistent with ext/curl.

To be honest, I don't really like it. But I'd only start thinking about the API if I manage to sort the rest of the problem out, specifically the issue with the C++ 17 dependency.

@iluuu1994
Copy link
Member

Note, adding support for URL parsing from curl was rejected a year ago. https://wiki.php.net/rfc/curl-url-api I'm also not sure if making curl a required dependency is going to fly. Unless I'm misunderstanding what you're saying.

@devnexen
Copy link
Member

specifically the issue with the C++ 17 dependency.

Are you aiming PHP 9 or earlier ?
Also, for example, would there a performance impact if you ditch string_view (no idea, real question)

@kocsismate
Copy link
Member Author

Are you aiming PHP 9 or earlier ?

Possibly earlier, but I'm not sure how we can resolve the C++ dependency issue. I mean, I don't see much sense to add Ada as an optional extension, as URL parsing is pretty much a core thing.

Also, for example, would there a performance impact if you ditch string_view (no idea, real question)

Me neither, although I'm not sure what you mean by ditching it? I don't use string_view. But @anonrig may be able to answer it. :)

@devnexen
Copy link
Member

devnexen commented Jun 22, 2023

Me neither, although I'm not sure what you mean by ditching it? I don't use string_view. But @anonrig may be able to answer it. :)

Yes true you do not use, but ada itself use typical useful C++17 features (e.g. std::optional).

@kocsismate
Copy link
Member Author

After a discussion with the PHP Foundation members, it turned out that we cannot yet rely on C++, not to mention C++ 17 indeed in case of mandatory extensions. That's why I won't continue to work on ext/url anymore since I believe such core features like URL parsing should always be available, and shouldn't be in optional extensions. That's why I'm closing this PR for now. Anyone who feels like continuing the effort should feel free to take it over.

@kocsismate kocsismate closed this Jul 18, 2023
@NattyNarwhal
Copy link
Member

Isn't the ICU extension C++ out of necessity? A more conservative dialect, but still C++.

@anonrig
Copy link

anonrig commented Jul 18, 2023

Isn't the ICU extension C++ out of necessity? A more conservative dialect, but still C++.

Ada doesn't depend on ICU

@NattyNarwhal
Copy link
Member

I was talking about ext/icu, WRT precedent.

@iluuu1994
Copy link
Member

iluuu1994 commented Jul 18, 2023

@NattyNarwhal Yes, but ext/intl is an optional extension, meaning you can still build PHP without intl and without C++.

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.

5 participants