-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Add ext/uri skeleton along with uriparser #18658
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
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.
Did not verify whether the uriparser files match the upstream sources.
zend_class_entry *uri_exception_ce; | ||
zend_class_entry *invalid_uri_exception_ce; | ||
zend_class_entry *whatwg_invalid_url_exception_ce; |
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.
These are not currently exported, but exporting the exception CEs might make sense. I suggest to rename them to avoid conflicts.
The following pattern is used by ext/random:
zend_class_entry *uri_exception_ce; | |
zend_class_entry *invalid_uri_exception_ce; | |
zend_class_entry *whatwg_invalid_url_exception_ce; | |
zend_class_entry *uri_ce_Uri_UriException; | |
zend_class_entry *uri_ce_Uri_InvalidUriException; | |
zend_class_entry *uri_ce_Uri_WhatWg_InvalidUrlException; |
see
Lines 58 to 72 in c7db07e
PHPAPI zend_class_entry *random_ce_Random_Engine; | |
PHPAPI zend_class_entry *random_ce_Random_CryptoSafeEngine; | |
PHPAPI zend_class_entry *random_ce_Random_Engine_Mt19937; | |
PHPAPI zend_class_entry *random_ce_Random_Engine_PcgOneseq128XslRr64; | |
PHPAPI zend_class_entry *random_ce_Random_Engine_Xoshiro256StarStar; | |
PHPAPI zend_class_entry *random_ce_Random_Engine_Secure; | |
PHPAPI zend_class_entry *random_ce_Random_Randomizer; | |
PHPAPI zend_class_entry *random_ce_Random_IntervalBoundary; | |
PHPAPI zend_class_entry *random_ce_Random_RandomError; | |
PHPAPI zend_class_entry *random_ce_Random_BrokenRandomEngineError; | |
PHPAPI zend_class_entry *random_ce_Random_RandomException; |
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.
Let's make renames a bit later when the actual content is also added - at least I can then use the IDE to also rename the references. 🤔
@TimWolla I just validated that uriparser matches upstream at commit 019af45951c76415bd7d999313faf5c0e53aea0d |
I'll sign off for today so I'll check tomorrow again (after CI is green) |
Ah, I've just realized that I have to add back the UriConfig.h that was a manually added file IIRC, because the CMake based build script would have to generate it by default. |
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.
LGTM.
Verified the uriparser sources against uriparser/uriparser@019af45 with the addition of the UriConfig.h
.
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.
One remark, otherwise lgtm
After the merge, the ability to build PHP in a directory different from the source dir is broken.
Please fix this ASAP. cc: @iluuu1994 @nielsdos |
Relates to #14461 and https://wiki.php.net/rfc/url_parsing_api