Everything

php

Suggested libgd ABI change leading to ext/gd BC break

php.internals

Christoph Becker

7 years ago

Hi!

Most recently, a pull request has been submitted for libgd[1], which
suggests to define image IO functions for several image formats
unconditionally; if the format is not supported, the IO functions would
fail with an error notice.  This would break the current ext/gd
configuration, since it relies on checks for the existence of these
functions[2] to define the respective PHP functions conditionally and
for introspection (e.g. gd_info()).  In other words, building ext/gd
against the PR would define imagecreatefromweb() even if WebP is not
supported.  This would break code relying on `if
(function_exists('imagecreatefromweb'))` or such.

We would have to change ext/gd/config.m4 accordingly to keep BC (we
could either rely on gdSupportsFileType() or actually call the functions
checking for the error message during build-time).  However, I wonder
about stable PHP versions, and particularly PHP 7.1 which is supposed to
receive only security updates.  If the libgd PR will be merged and
released with libgd 2.3.0 which may well happen during this year,
unmodified PHP versions would break BC if built against libgd 2.3.0.

Suggestions how to deal with this are welcome!

[1] <https://github.com/libgd/libgd/pull/491>
[2]
<https://github.com/php/php-src/blob/php-7.3.1/ext/gd/config.m4#L217-L225>

--
Christoph M. Becker

Zeev Suraski

7 years ago

> On 15 Jan 2019, at 13:13, Christoph M. Becker <cmbecker69@gmx.de> wrote:
>
> Hi!
>
> Most recently, a pull request has been submitted for libgd[1], which
> suggests to define image IO functions for several image formats
> unconditionally; if the format is not supported, the IO functions would
> fail with an error notice.  This would break the current ext/gd
> configuration, since it relies on checks for the existence of these
> functions[2] to define the respective PHP functions conditionally and
> for introspection (e.g. gd_info()).  In other words, building ext/gd
> against the PR would define imagecreatefromweb() even if WebP is not
> supported.  This would break code relying on `if
> (function_exists('imagecreatefromweb'))` or such.

Given this downside, what is the upside of introducing this change?

Zeev

Christoph Becker

7 years ago

On 15.01.2019 at 13:32, Zeev Suraski wrote:

>> On 15 Jan 2019, at 13:13, Christoph M. Becker <cmbecker69@gmx.de> wrote:
>>
>> Most recently, a pull request has been submitted for libgd[1], which
>> suggests to define image IO functions for several image formats
>> unconditionally; if the format is not supported, the IO functions would
>> fail with an error notice.  This would break the current ext/gd
>> configuration, since it relies on checks for the existence of these
>> functions[2] to define the respective PHP functions conditionally and
>> for introspection (e.g. gd_info()).  In other words, building ext/gd
>> against the PR would define imagecreatefromweb() even if WebP is not
>> supported.  This would break code relying on `if
>> (function_exists('imagecreatefromweb'))` or such.
>
> Given this downside, what is the upside of introducing this change?

Basically, to be able to switch libgd.so without recompiling its
clients, see
<https://github.com/libgd/libgd/issues/345#issuecomment-454148489>.  Of
course, that wouldn't work for ext/gd as it is now, but that is a
different story.  We have to deal with whatever the libgd developers decide.

--
Christoph M. Becker

Zeev Suraski

7 years ago

Sent from my mobile
On 15 Jan 2019, at 15:29, Christoph M. Becker <cmbecker69@gmx.de<mailto:cmbecker69@gmx.de>> wrote:

On 15.01.2019 at 13:32, Zeev Suraski wrote:

On 15 Jan 2019, at 13:13, Christoph M. Becker <cmbecker69@gmx.de<mailto:cmbecker69@gmx.de>> wrote:

Most recently, a pull request has been submitted for libgd[1], which
suggests to define image IO functions for several image formats
unconditionally; if the format is not supported, the IO functions would
fail with an error notice.  This would break the current ext/gd
configuration, since it relies on checks for the existence of these
functions[2] to define the respective PHP functions conditionally and
for introspection (e.g. gd_info()).  In other words, building ext/gd
against the PR would define imagecreatefromweb() even if WebP is not
supported.  This would break code relying on `if
(function_exists('imagecreatefromweb'))` or such.

Given this downside, what is the upside of introducing this change?

Basically, to be able to switch libgd.so without recompiling its
clients, see
<https://github.com/libgd/libgd/issues/345#issuecomment-454148489>.  Of
course, that wouldn't work for ext/gd as it is now, but that is a
different story.  We have to deal with whatever the libgd developers decide.


If that's the reason - I believe it should be possible to retain the current behavior and also get the benefit of being able to replace the underlying library.  I think we can just register the relevant functions dynamically in MINIT based on what's available in runtime.

I've seen the pattern of using function_exists() to determine whether or not a given format is supported in numerous places over the years, so I think we should avoid breaking it.

Zeev