Skip to content

Add ext/uri skeleton along with uriparser - #18658

Merged
kocsismate merged 10 commits into
php:masterfrom
kocsismate:ext-url4
May 27, 2025
Merged

Add ext/uri skeleton along with uriparser#18658
kocsismate merged 10 commits into
php:masterfrom
kocsismate:ext-url4

Conversation

@kocsismate

Copy link
Copy Markdown
Member

Comment threadext/uri/php_uri.c Outdated
Comment threadext/uri/php_uri.c Outdated
Comment threadext/uri/php_uri.c Outdated

@TimWollaTimWolla left a comment

Copy link
Copy Markdown
Member

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.

Comment threadext/uri/php_uri.h Outdated
Comment threadext/uri/php_uri.c Outdated
Comment threadext/uri/php_uri.c Outdated
Comment threadext/uri/php_uri.c
Comment on lines +30 to +32
zend_class_entry *uri_exception_ce;
zend_class_entry *invalid_uri_exception_ce;
zend_class_entry *whatwg_invalid_url_exception_ce;

Copy link
Copy Markdown
Member

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:

Suggested change
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

PHPAPIzend_class_entry*random_ce_Random_Engine;
PHPAPIzend_class_entry*random_ce_Random_CryptoSafeEngine;
PHPAPIzend_class_entry*random_ce_Random_Engine_Mt19937;
PHPAPIzend_class_entry*random_ce_Random_Engine_PcgOneseq128XslRr64;
PHPAPIzend_class_entry*random_ce_Random_Engine_Xoshiro256StarStar;
PHPAPIzend_class_entry*random_ce_Random_Engine_Secure;
PHPAPIzend_class_entry*random_ce_Random_Randomizer;
PHPAPIzend_class_entry*random_ce_Random_IntervalBoundary;
PHPAPIzend_class_entry*random_ce_Random_RandomError;
PHPAPIzend_class_entry*random_ce_Random_BrokenRandomEngineError;
PHPAPIzend_class_entry*random_ce_Random_RandomException;

Copy link
Copy Markdown
MemberAuthor

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. 🤔

Comment threadext/uri/CREDITS
Comment threadext/uri/config.m4 Outdated
Comment threadext/uri/config.m4 Outdated
@ndossche

Copy link
Copy Markdown
Member

@TimWolla I just validated that uriparser matches upstream at commit 019af45951c76415bd7d999313faf5c0e53aea0d

@ndossche

Copy link
Copy Markdown
Member

I'll sign off for today so I'll check tomorrow again (after CI is green)

@kocsismate

Copy link
Copy Markdown
MemberAuthor

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.

@TimWollaTimWolla left a comment

Copy link
Copy Markdown
Member

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.

@ndosschendossche left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

One remark, otherwise lgtm

Comment threadext/uri/config.w32 Outdated
@kocsismate
kocsismate merged commit d585a56 into php:masterMay 27, 2025
@kocsismate
kocsismate deleted the ext-url4 branch May 27, 2025 06:16
@dstogov

dstogov commented Jun 2, 2025

Copy link
Copy Markdown
Member

After the merge, the ability to build PHP in a directory different from the source dir is broken.

$ buildconf --force
$ mkdir -p CGI-RELEASE-64
$ cd CGI-RELEASE-64
$ ../configure ...
$ make clean; make -j8
...
/bin/sh /home/dmitry/php/php-master/CGI-RELEASE-64/libtool --silent --preserve-dup-deps --tag=CC --mode=compile cc -Iext/uri/ -I/home/dmitry/php/php-master/ext/uri/ -I/home/dmitry/php/php-master/CGI-RELEASE-64/main -I/home/dmitry/php/php-master/CGI-RELEASE-64 -I/home/dmitry/php/php-master/main -I/home/dmitry/php/php-master -I/usr/include/valgrind -I/home/dmitry/php/php-master/CGI-RELEASE-64/ext/date/lib -I/home/dmitry/php/php-master/ext/date/lib -I/usr/include/libxml2 -I/usr/include/enchant-2 -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/sysprof-6 -I/usr/include/libpng16 -I/usr/include/freetype2 -I/usr/include/harfbuzz -I/home/dmitry/php/php-master/ext/lexbor -I/home/dmitry/php/php-master/ext/mbstring/libmbfl -I/home/dmitry/php/php-master/CGI-RELEASE-64/ext/mbstring/libmbfl -I/home/dmitry/php/php-master/ext/mbstring/libmbfl/mbfl -I/home/dmitry/php/php-master/CGI-RELEASE-64/ext/mbstring/libmbfl/mbfl -I/usr/include/capstone -I/home/dmitry/php/php-master/CGI-RELEASE-64/TSRM -I/home/dmitry/php/php-master/CGI-RELEASE-64/Zend -I/home/dmitry/php/php-master/Zend -I/home/dmitry/php/php-master/TSRM -D_GNU_SOURCE -fno-common -Wstrict-prototypes -Wformat-truncation -Wlogical-op -Wduplicated-cond -Wno-clobbered -Wall -Wextra -Wno-unused-parameter -Wno-sign-compare -g -O2 -ffp-contract=off -fvisibility=hidden -DNDEBUG -Wimplicit-fallthrough=1 -DZEND_SIGNALS -Iext/uri/uriparser/include -DURI_STATIC_BUILD -DZEND_ENABLE_STATIC_TSRMLS_CACHE=1 -c /home/dmitry/php/php-master/ext/uri/uriparser/src/UriNormalize.c -o ext/uri/uriparser/src/UriNormalize.lo -MMD -MF ext/uri/uriparser/src/UriNormalize.dep -MT ext/uri/uriparser/src/UriNormalize.lo
/home/dmitry/php/php-master/ext/uri/uriparser/src/UriCommon.c:41:10: fatal error: uriparser/UriDefsConfig.h: No such file or directory
41 | #include <uriparser/UriDefsConfig.h>
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.

Please fix this ASAP.

cc: @iluuu1994@nielsdos

@kocsismate

Copy link
Copy Markdown
MemberAuthor

@dstogov Arnaud has already created a PR to fix this: #18716 I'm going to merge it now.

Sign up for freeto 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.

4 participants

@kocsismate@ndossche@dstogov@TimWolla