Skip to content

Conversation

remicollet
Copy link
Member

To avoid another bundled library

Test suite with version 0.9.8 passes

Notice: It seems the bundled version is not really 0.9.8, but rather a git snapshot

New build option --with-external-uriparser (as we already have --with-external-gd and --with-external-libcrypt)
Use bundled sources by default.

@TimWolla
Copy link
Member

Test suite with version 0.9.8 passes

That's likely because the library is not actually used yet. See #18836. The current git HEAD of uriparser contains some fixes contributed by Máté.

@remicollet
Copy link
Member Author

That's likely because the library is not actually used yet

:(

BTW, I think we should allow building with system libraries, and bundled libraries should be exceptional

It seems a new version (0.9.9) will required (and we should encourage upstream to release it with needed patches)

@TimWolla
Copy link
Member

It seems a new version (0.9.9) will required

Yes.

(and we should encourage upstream to release it with needed patches)

Yes, once all the necessary changes are included upstream.

@kocsismate
Copy link
Member

It seems a new version (0.9.9) will required (and we should encourage upstream to release it with needed patches)

Yes, 0.9.9 will likely be the next release for sure according to this review suggestion: uriparser/uriparser#230 (comment)

@TimWolla
Copy link
Member

TimWolla commented Sep 4, 2025

@remicollet You can update this one after #19711 is merged.

@remicollet
Copy link
Member Author

@remicollet You can update this one after #19711 is merged.

rebased and version bump to 0.9.9

Tested locally with system liburiparser 0.9.9
Test suite passes

Copy link
Member

@TimWolla TimWolla left a comment

Choose a reason for hiding this comment

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

Looks correct. Requesting review from Máté and the RMs.

@TimWolla TimWolla requested review from kocsismate and a team September 5, 2025 07:30
@TimWolla
Copy link
Member

TimWolla commented Sep 5, 2025

Test suite passes

@remicollet Note that the setters will only arrive with #19636, but I don't expect any further upstream patches.

Copy link
Member

@DanielEScherzer DanielEScherzer left a comment

Choose a reason for hiding this comment

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

RM approval, technical review not performed, noticed what I think is a typo if you want to address it

PHP_ARG_WITH([external-uriparser],
[for external/system liburiparser],
[AS_HELP_STRING([--with-external-uriparser],
[Use external system liburiparser])],
Copy link
Member

Choose a reason for hiding this comment

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

nit: others use "external" only:

  [AS_HELP_STRING([--with-external-gd],
    [Use external libgd])],

This seems correct, because it might be the system one (it might not), but it's definitely external.

@remicollet
Copy link
Member Author

@TimWolla please review again for you change in phpinfo

@remicollet remicollet requested a review from TimWolla September 5, 2025 12:56
Copy link
Member

@TimWolla TimWolla left a comment

Choose a reason for hiding this comment

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

Thanks, that makes sense to me.

@remicollet remicollet merged commit 66eb5e7 into php:master Sep 5, 2025
9 checks passed
@remicollet
Copy link
Member Author

Thanks for the reviews

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