Skip to content

Conversation

kocsismate
Copy link
Member

No description provided.

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.

You are not updating src/UriConfig.h, thus a wrong version number will be reported. Please review #19678 first.

@kocsismate
Copy link
Member Author

You are not updating src/UriConfig.h, thus a wrong version number will be reported. Please review #19678 first.

Ah... I had the impression that you are going to remove this file 🤔 But let me have a look at it

@TimWolla
Copy link
Member

TimWolla commented Sep 4, 2025

Ah... I had the impression that you are going to remove this file 🤔

You can now remove it as part of this PR after a rebase for a clean upgrade.

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.

I have verified the changes in this PR against a fresh clone of the upstream repository. The C and H files are in sync, but I'm noticing that you did not update the COPYING files.

Both src/ and include/ now have dedicated COPYING files. The one directly in ext/uri/uriparser should be removed and replaced by the COPYING.BSD-3-Clause.

@kocsismate
Copy link
Member Author

Yes, I noticed it as well, but does it matter? I mean, what is the difference between the two options?

P.S. We'll need the UriConfig file

@TimWolla
Copy link
Member

TimWolla commented Sep 4, 2025

Ah and CI fails, because the UriConfig.h was actually required by the code itself.

@TimWolla
Copy link
Member

TimWolla commented Sep 4, 2025

Yes, I noticed it as well, but does it matter? I mean, what is the difference between the two options?

I consider it important to replicate the upstream repository as close as possible to make updates easier (e.g. just replacing the entire include/ and src/ directories). Also for licensing it would try to very closely follow upstream, so that we do not accidentally get out of sync and claim incorrect information.

@TimWolla
Copy link
Member

TimWolla commented Sep 4, 2025

The one directly in ext/uri/uriparser should be removed and replaced by the COPYING.BSD-3-Clause.

Please also this one: https://github.com/uriparser/uriparser/blob/master/COPYING.BSD-3-Clause for completeness. The other two in the top directory do not matter, since they are for tests and fuzzing code.

@kocsismate kocsismate merged commit 01ae278 into php:master Sep 4, 2025
9 checks passed
@kocsismate kocsismate deleted the ext-url9 branch September 4, 2025 21:58
@mvorisek mvorisek mentioned this pull request Sep 11, 2025
7 tasks
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.

2 participants