Skip to content

Remove confusing GD version info #3080

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Conversation

cmb69
Copy link
Member

@cmb69 cmb69 commented Feb 6, 2018

Since the bundled libgd is actually a partially unsynchronized fork of
the upstream project, it does not make sense to use their version
numbers. Instead of introducing our own version numbers, we don't even
define GD_VERSION, GD_MAJOR_VERSION, GD_MINOR_VERSION,
GD_RELEASE_VERSION and GD_EXTRA_VERSION anymore. If users need to
check the version, they could use GD_BUNDLED and the PHP version info
– what's likely the only reasonable thing to do already.

We also simplify phpinfo() and gd_info() to say "bundled" without
any (erroneous) compatibility statement.

See also https://externals.io/message/101744.

Since the bundled libgd is actually a partially unsynchronized fork of
the upstream project, it does not make sense to use their version
numbers.  Instead of introducing our own version numbers, we don't even
define `GD_VERSION`, `GD_MAJOR_VERSION`, `GD_MINOR_VERSION`,
`GD_RELEASE_VERSION` and `GD_EXTRA_VERSION` anymore.  If users need to
check the version, they could use `GD_BUNDLED` and the PHP version info
– what's likely the only reasonable thing to do already.

We also simplify `phpinfo()` and `gd_info()` to say "bundled" without
any (erroneous) compatibility statement.

See also https://externals.io/message/101744.
@hikari-no-yume
Copy link
Contributor

This is… significantly more reasonable than my silly suggestion, I approve of it :)

@KalleZ
Copy link
Member

KalleZ commented Feb 6, 2018

+1, tho I think it's time we solve the bundled libgd issue once and for all

@cmb69
Copy link
Member Author

cmb69 commented Feb 7, 2018

[…] tho I think it's time we solve the bundled libgd issue once and for all

Yep, ideally we should bundle unmodified upstream libgd (if bundling would even be necessary). The problem: there are still several minor (and maybe some not so minor) differences, and the PHP manual recommends to use the bundled libgd. If we simply switch to the upstream lib, we'd cause a BC. (One of the minor differences: libgd/libgd#298.)

@remicollet
Copy link
Member

+1 for this change

Yep, ideally we should bundle unmodified upstream libgd (if bundling would even be necessary).

+1, and +1 to drop bundled lib later (as for other extension, openssl, curl, intl, ...) no bundled library should be the standard

If we simply switch to the upstream lib, we'd cause a BC.

BTW, linux distributions already commonly use the system library (packaging Guidelines), at least, Debian, Fedora, and derivatives.

@nikic
Copy link
Member

nikic commented Feb 7, 2018

I don't like this change, because it will remove previously existing constants. Even if not super meaningful, just dropping them is going to make any existing code using them throw notices. If that's the goal, then they should be removed entirely, also for the non-bundled library.

@cmb69
Copy link
Member Author

cmb69 commented Feb 7, 2018

BTW, linux distributions already commonly use the system library (packaging Guidelines), at least, Debian, Fedora, and derivatives.

Yes, I know. But still, the PHP manual recommends to use the bundled libgd. At least some users might do that deliberately … If we don't see this as an issue, I can author an RFC to bundle upstream libgd for PHP 7.3 (but still there is libgd/libgd@335).

If that's the goal, then they should be removed entirely, also for the non-bundled library.

For upstream libgd the constants might be important, due to potential implementation changes. See, for instance, https://bugs.php.net/73291. Using imagecropauto($im, IMG_CROP_THRESHOLD) in a portable way, presently requires to apply different thresholds depending on GD_BUNDLED. If the implementation of upstream libgd will be changed there should be a possibility to check the version (otherwise one would have to do a test crop behind the scenes).

@nikic
Copy link
Member

nikic commented Feb 7, 2018

Yes, I know. But still, the PHP manual recommends to use the bundled libgd. At least some users might do that deliberately … If we don't see this as an issue, I can author an RFC to bundle upstream libgd for PHP 7.3 (but still there is libgd/libgd@335).

I'd be in favor of doing this, even if there are some BC breaks. The current state where the bundled and system libraries (in practice: the Windows and Linux implementations) differ in behavior is something we'll want to fix at some point, and I don't think it's something that warrants holding off for a major version especially given the current state where you effectively must handle both behaviors if there are discrepancies, as both setups are common.

@cmb69
Copy link
Member Author

cmb69 commented Feb 7, 2018

@nikic I've mailed internals.

@cmb69
Copy link
Member Author

cmb69 commented Feb 8, 2018

Since this PR would be obsolete, if we'll bundle upstream libgd for PHP 7.3, I'm closing. I'll reopen, if that won't happen.

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.

6 participants