ext/gd: de-factorize image output functions #14523
Conversation
thg2k
force-pushed
the
pr/gd_out_refactor_2
branch
from
a465d90 to
0db0d51
Compare
ext/gd/gd.c
Outdated
| if (quality < -1 || quality > 100) { | ||
| zend_argument_value_error(3, "must be between -1 and 100"); | ||
| ctx->gd_free(ctx); | ||
| RETURN_THROWS(); | ||
| } | ||
| if (speed < -1 || speed > 10) { | ||
| zend_argument_value_error(4, "must be between -1 and 10"); | ||
| ctx->gd_free(ctx); | ||
| RETURN_THROWS(); | ||
| } else if (speed == -1) { | ||
| speed = 6; | ||
| } |
There was a problem hiding this comment.
Ditto those checks should be before the call to php_gd_libgdimageptr_from_zval_p
| if (quality < -1 || quality > 100) { | ||
| zend_argument_value_error(3, "must be at between -1 and 100"); | ||
| ctx->gd_free(ctx); | ||
| RETURN_THROWS(); | ||
| } |
| ctx->gd_free(ctx); | ||
| RETURN_THROWS(); | ||
| if (Z_TYPE_P(to_zval) == IS_RESOURCE) { | ||
| php_stream_from_zval_no_verify(stream, to_zval); |
There was a problem hiding this comment.
Why is this no verify? This function should return a TypeError for incorrect resources.
There was a problem hiding this comment.
I only moved around this code, so I have no idea why it's no verify. This PR is only about reorganizing the file, it should not contain any functional changes.
There was a problem hiding this comment.
Okay, I'll keep a note for a follow-up then.
thg2k
force-pushed
the
pr/gd_out_refactor_2
branch
from
0db0d51 to
31c4d5a
Compare
|
I rebased it due to #14534 and added the argument number as parameter |
The image output functions imagegif/imagepng/imagewebp/imageavif/imagejpeg were all calling the static function _php_image_output_ctx which was basically a big switch statement between each image type which also have different parameters. The only identical part was the call to create_stream_context_from_zval that is now merged with create_output_context.
thg2k
force-pushed
the
pr/gd_out_refactor_2
branch
from
31c4d5a to
df54879
Compare
Sure thing, I added the assertion. From the procedural point of views, in this case that the PR was double approved already, is it better if I amend the first commit or just add a second one and you guys squash it? I think rebasing is out of the question because it's hard to check the changes.. Just to know how to do it in the future 😄 |
|
does not matter you can squash it or the committer will ;) |
The image output functions imagegif/imagepng/imagewebp/imageavif/imagejpeg were all calling the static function _php_image_output_ctx which was basically a big switch statement between each image type which also have different parameters.
The only identical part was the call to create_stream_context_from_zval that is now merged with create_output_context.
This greatly increases readability and maintainability of this file.