Skip to content

uri: Improve exceptions for Uri\Rfc3986\Uri - #19161

Merged
TimWolla merged 6 commits into
php:masterfrom
TimWolla:uri-uriparser-parse-simplify
Jul 18, 2025
Merged

uri: Improve exceptions for Uri\Rfc3986\Uri#19161
TimWolla merged 6 commits into
php:masterfrom
TimWolla:uri-uriparser-parse-simplify

Conversation

@TimWolla

@TimWolla TimWolla commented Jul 17, 2025

Copy link
Copy Markdown
Member

No description provided.

@TimWolla
TimWolla requested review from kocsismate and ndossche July 17, 2025 17:22
TimWolla
TimWolla commented Jul 17, 2025
View reviewed changes
Comment thread ext/uri/php_uriparser.c Outdated
@TimWolla
TimWolla force-pushed the uri-uriparser-parse-simplify branch from 3467d7c to 5045210 Compare July 17, 2025 17:24
@TimWolla

This comment was marked as resolved.

Sign in to view
ndossche
ndossche approved these changes Jul 17, 2025
View reviewed changes
Comment thread ext/uri/php_uriparser.c Outdated
Comment thread ext/uri/php_uriparser.c Outdated
@TimWolla
TimWolla force-pushed the uri-uriparser-parse-simplify branch from 5045210 to 7e2ba9d Compare July 17, 2025 18:45
@ndossche

ndossche commented Jul 17, 2025

Copy link
Copy Markdown
Member

ASAN is green, but the others are not. Probably missed a Mm somewhere. Will check later.

Ah I missed this. I have a habit of always running with ASAN but I guess I should use USE_TRACKED_ALLOC more often. Funny that this is a case where ASAN hides the issue.

@TimWolla

TimWolla commented Jul 17, 2025

Copy link
Copy Markdown
Member Author

Ah I missed this. I have a habit of always running with ASAN

Same on both ends. But already fixed.

kocsismate
kocsismate reviewed Jul 18, 2025
View reviewed changes
Comment thread ext/uri/php_uriparser.c
kocsismate
kocsismate reviewed Jul 18, 2025
View reviewed changes
Comment thread ext/uri/php_uriparser.c
@kocsismate

kocsismate commented Jul 18, 2025

Copy link
Copy Markdown
Member

Ah I missed this. I have a habit of always running with ASAN but I guess I should use USE_TRACKED_ALLOC more often. Funny that this is a case where ASAN hides the issue.

Yeah, I always bump into this problem that ASAN hides the memleaks. "Fortunately", I somehow didn't manage to setup ASAN for the Clion debugger/runner, so I can reproduce leaks by running tests one by one from CLion.

@TimWolla
TimWolla requested a review from kocsismate July 18, 2025 13:34
@TimWolla

TimWolla commented Jul 18, 2025

Copy link
Copy Markdown
Member Author

@kocsismate Updated to allow empty URIs. PTAL, if this is good to you, I'll merge myself after CI is green 😄

@TimWolla

TimWolla commented Jul 18, 2025

Copy link
Copy Markdown
Member Author

Yeah, I always bump into this problem that ASAN hides the memleaks.

ASAN should report memleaks by itself (it just doesn't catch incorrect usage of ZendMM).

kocsismate
kocsismate reviewed Jul 18, 2025
View reviewed changes
Comment thread ext/uri/tests/004.phpt Outdated
kocsismate
kocsismate reviewed Jul 18, 2025
View reviewed changes

@kocsismate kocsismate 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 after the nit is fixed.

@TimWolla
TimWolla force-pushed the uri-uriparser-parse-simplify branch from 559529c to 48f1824 Compare July 18, 2025 13:42
@TimWolla TimWolla mentioned this pull request Jul 18, 2025
Merged
@TimWolla
TimWolla merged commit 5fdc022 into php:master Jul 18, 2025
@TimWolla
TimWolla deleted the uri-uriparser-parse-simplify branch July 18, 2025 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kocsismate kocsismate kocsismate left review comments
@ndossche ndossche ndossche approved these changes

Assignees

No one assigned

Labels

Extension: uri

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@TimWolla @ndossche @kocsismate