Skip to content

Conversation

cmb69
Copy link
Member

@cmb69 cmb69 commented Jul 24, 2016

We add PHP bindings for libgd's features to read and write BMP files, which
are available as of libgd 2.1.0.

As PHP's bundled libgd doesn't yet include the respective features of the
external libgd, we add these.

We add PHP bindings for libgd's features to read and write BMP files, which
are available as of libgd 2.1.0.

As PHP's bundled libgd doesn't yet include the respective features of the
external libgd, we add these.
@cmb69
Copy link
Member Author

cmb69 commented Jul 29, 2016

Any objections to apply this PR to master? Or should it be applied to PHP 7.1+? Either way, the patch would also be a small step to sync PHP's bundled libgd. @pierrejoye, @dshafik.

@pierrejoye
Copy link
Contributor

I am fine, thanks :)

On Jul 29, 2016 5:03 PM, "Christoph M. Becker" notifications@github.com
wrote:

Any objections to apply this PR to master? Or should it be applied to PHP
7.1+? Either way, the patch would also be a small step to sync PHP's
bundled libgd. @pierrejoye https://github.com/pierrejoye, @dshafik
https://github.com/dshafik.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#2029 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AARPKKlqe227PieTX0Va5s-Q1sljwnTWks5qadALgaJpZM4JTkPF
.

@@ -2738,6 +2783,16 @@ PHP_FUNCTION(imagegd2)
}
/* }}} */

#ifdef HAVE_GD_BMP
/* {{{ proto bool imagebmp(resource im [, string filename [, int compression]])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think $compression should be 'boolean' here (from reading the source of gdImageBmpCtx())

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! Indeed $compression is treated as bool, but actually imagebmp() accepts an int. Wrt. strict_types probably _php_image_output_ctx() has to be changed to have an own ZPP for PHP_GDIMG_TYPE_BMP.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah originally about the "q != 0" part, I would probably have gone with something like "!!q" to let C decide on its boolean conversion value, but that should have no impact if ZPP is changed ;-)

@KalleZ
Copy link
Member

KalleZ commented Jul 29, 2016

Huge +1, so we can have a synced libgd in PHP

@KalleZ
Copy link
Member

KalleZ commented Aug 2, 2016

@cmb69 We might also look into adding TIFF+TGA support which seems to be in libgd, at least in master.

@pierrejoye I take it as those implementations are finale (TGA+TIFF)?

@cmb69
Copy link
Member Author

cmb69 commented Aug 2, 2016

We might also look into adding TIFF+TGA support which seems to be in libgd, at least in master.

I've noticed just today that the bundled libgd doesn't have TIFF support yet, so I'm already planning a respective PR. And yeah, TGA support should also be added.

@pierrejoye
Copy link
Contributor

Please no tiff yet. This is still experimental, no multi page support
(defeats the whole tiff usage).

Ok witg tga.

Also please keep in mind we need simply to sync 2.2.x to 7.1 branch, with
or without exposing new features.

On Aug 2, 2016 10:38 PM, "Christoph M. Becker" notifications@github.com
wrote:

We might also look into adding TIFF+TGA support which seems to be in
libgd, at least in master.

I've noticed just today that the bundled libgd doesn't have TIFF support
yet, so I'm already planning a respective PR. And yeah, TGA support should
also be added.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#2029 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AARPKMmGxQS1QGGdV_rO1CfhsJ0OYYguks5qb2SKgaJpZM4JTkPF
.

@cmb69
Copy link
Member Author

cmb69 commented Aug 2, 2016

Please no tiff yet.

Okay. :-)

@krakjoe
Copy link
Member

krakjoe commented Jan 3, 2017

@cmb69 please fix merge conflicts, and merge into master.

@php-pulls
Copy link

Comment on behalf of cmb at php.net:

Applied via commit 500b496.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants