Fix GH-17703: imagescale both width and heigh set with negative values.#17708
Fix GH-17703: imagescale both width and heigh set with negative values.#17708devnexen wants to merge 1 commit intophp:PHP-8.4from
Conversation
cmb69
left a comment
cmb69
left a comment
There was a problem hiding this comment.
Besides the nit below, this looks good to me. Thank you!
ext/gd/gd.c
Outdated
| im = php_gd_libgdimageptr_from_zval_p(IM); | ||
|
|
||
| if (tmp_h < 0 && tmp_w < 0) { | ||
| zend_value_error("$(width) and $(height) cannot be both negative"); |
There was a problem hiding this comment.
I don't think the parens make sense here:
| zend_value_error("$(width) and $(height) cannot be both negative"); | |
| zend_value_error("$width and $height cannot be both negative"); |
ndossche
left a comment
ndossche
left a comment
There was a problem hiding this comment.
Seems fine for master, but why do this on lower branches? Now people get an exception instead of returning false, no?
ext/gd/gd.c
Outdated
| im = php_gd_libgdimageptr_from_zval_p(IM); | ||
|
|
||
| if (tmp_h < 0 && tmp_w < 0) { | ||
| zend_value_error("$width and $height cannot be both negative"); |
There was a problem hiding this comment.
We typically construct an argument_value_error-esque wording when doing these kinds of things.
Something like: Argument #2 ($width) and argument #3 ($height) cannot both be negative.
There was a problem hiding this comment.
Ok will change to return false here instead and will do a PR just for master, wdyt ?
There was a problem hiding this comment.
I think the patch as it's now doesn't change anything (well, we bail out a bit earlier), since previously the function also returned false if both width and height have been negative. I suggest to apply the previous patch to PHP-8.4+ (the ValueError was already there previously, only the error message changes). Only applying that to master is fine for me, too.
…lues. Throwing a ValueError in this particular case.
devnexen
requested review from
Girgias,
SakiTakamachi,
TimWolla,
adoy,
alexdowad,
arnaud-lb,
bukka,
derickr,
dstogov,
kocsismate and
youkidearitai
as code owners
devnexen
removed request for
Girgias,
TimWolla,
adoy,
alexdowad,
arnaud-lb,
bukka,
derickr,
dstogov,
kocsismate and
youkidearitai
ndossche
left a comment
ndossche
left a comment
There was a problem hiding this comment.
Sorry for the small review hiccup, looks good now!
3 participants
Throwing a ValueError in this particular case.