Skip to content

ext/gd: Add ZPP specifiers for array|float parameters - #23384

Open
LamentXU123 wants to merge 3 commits into
php:masterfrom
LamentXU123:zpp
Open

ext/gd: Add ZPP specifiers for array|float parameters#23384
LamentXU123 wants to merge 3 commits into
php:masterfrom
LamentXU123:zpp

Conversation

@LamentXU123

@LamentXU123 LamentXU123 commented Aug 19, 2026

Copy link
Copy Markdown
Member

Follow-up #23356 (comment)_ cc @Girgias

@Girgias

Girgias commented Aug 19, 2026

Copy link
Copy Markdown
Member

I was thinking a custom one for the GD extension defined in the GD extension. As I don't think any other extension is going to utilize this.

@LamentXU123
LamentXU123 marked this pull request as draft August 19, 2026 15:34
@LamentXU123 LamentXU123 changed the title Zend: Add ZPP specifiers for array|float parameters ext/gd: Add ZPP specifiers for array|float parameters Aug 19, 2026
@LamentXU123
LamentXU123 marked this pull request as ready for review August 19, 2026 15:43
@LamentXU123
LamentXU123 requested a review from devnexen as a code owner August 19, 2026 15:43
@LamentXU123

Copy link
Copy Markdown
Member Author

Either way make sense to me.

Also I am aware the old test file doesn't test exception types. Don't need to change them in this PR if we want to change them we can do it in bulk.

@Girgias Girgias 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.

Minor nit but LGTM

Comment thread ext/gd/gd.c Outdated
Comment thread ext/gd/gd.c

if ((tmp = zend_hash_str_find(Z_ARRVAL_P(options), "x", sizeof("x") - 1)) != NULL) {
if ((tmp = zend_hash_str_find(options, "x", sizeof("x") - 1)) != NULL) {
x = zval_get_double(tmp);

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.

Future PR: Ideally this should use a stricter variant as tmp could be an object of an array and still pass.

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