-
Notifications
You must be signed in to change notification settings - Fork 7.8k
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
POC: Add ext/url #11315
Conversation
@@ -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 |
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.
I only removed this just to be able to compare its real performance
e67f6bb
to
9365001
Compare
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. |
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. |
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. |
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.
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). |
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. |
Isn't the ICU extension C++ out of necessity? A more conservative dialect, but still C++. |
Ada doesn't depend on ICU |
I was talking about |
@NattyNarwhal Yes, but ext/intl is an optional extension, meaning you can still build PHP without intl and without C++. |
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.