Skip to content
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

ext/gd/tests: external gd-2.3.3 compatibility. #11262

Closed
wants to merge 1 commit into from

Conversation

orlitzky
Copy link
Contributor

Support for the legacy "gd" image format was removed from gd-2.3.3 upstream:

https://github.com/libgd/libgd/blob/master/CHANGELOG.md#233---2021-09-12

Several tests for the gd extension utilize that format, and naturally fail when gd-2.3.3 from the system is used. This commit skips those tests when the version of gd is at least 2.3.3.

Support for the legacy "gd" image format was removed from gd-2.3.3
upstream:

  https://github.com/libgd/libgd/blob/master/CHANGELOG.md#233---2021-09-12

Several tests for the gd extension utilize that format, and naturally
fail when gd-2.3.3 from the system is used. This commit skips those
tests when the version of gd is at least 2.3.3.
--SKIPIF--
<?php
if (!GD_BUNDLED && version_compare(GD_VERSION, '2.3.3', '>=')) {
die("skip test requires GD 2.3.2 or older");
Copy link
Member

@Girgias Girgias May 20, 2023

Choose a reason for hiding this comment

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

Suggested change
die("skip test requires GD 2.3.2 or older");
die("skip test requires a version of GD older than 2.3.3");

As the current wording is kinda weird IMHO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have a few pull requests open with this wording and the underlying issue (#11252) isn't quite done yet. Should I change them all now, or would it be easier to sed them afterwards?

Copy link
Member

Choose a reason for hiding this comment

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

If it's easy for you to do now, that would be convenient.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On second thought, it looks like I chose this wording to match the existing version-based "skip" messages. For example, https://github.com/php/php-src/blob/master/ext/gd/tests/bug43121.phpt#L8

That same phrase (with various version triplets) already appears 25 times in ext/gd/tests. I'm still happy to change it though if this doesn't change anyone's mind.

Copy link
Member

Choose a reason for hiding this comment

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

I see. IMO if it's already this way anyway, it can be kept. It's not that important after all.
If we end up disliking the wording I can simply amend it later.

Copy link
Member

Choose a reason for hiding this comment

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

Right, if it's already liek this leave it then

Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

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

Not sure if we should backport the changes or not

@nielsdos nielsdos closed this in 0aaad46 Jul 6, 2023
@cmb69
Copy link
Member

cmb69 commented Jul 17, 2024

Support for the legacy "gd" image format was removed from gd-2.3.3 upstream:

https://github.com/libgd/libgd/blob/master/CHANGELOG.md#233---2021-09-12

The changelog says: "support for the legacy/testing gd image formats is disabled by default". It is perfectly possible to still build libgd 2.3.3 with support for these formats (otherwise that would have been a serious BC break). ext/gd/tests/bug41442.phpt, for instance, wouldn't even need to have been touched, since the test has already been skipped, if imagegd2() is not available.

@nielsdos
Copy link
Member

@cmb69 Okay, I can open a PR to change the check to check if imagegd2 is defined instead, sounds good?

@cmb69
Copy link
Member

cmb69 commented Jul 17, 2024

@nielsdos, I think checking for imagegd2() (or imagegd()) should do, but I'm not up to date with the extension, and can have a look at it (might take a day or three). Particularly, I wonder whether gd_info() already reports gd/gd2 support.

@nielsdos
Copy link
Member

@cmb69 gd_info does not report gd2 file format support (

php-src/ext/gd/gd.c

Lines 472 to 521 in 1b07772

#ifdef HAVE_GD_FREETYPE
add_assoc_bool(return_value, "FreeType Support", 1);
add_assoc_string(return_value, "FreeType Linkage", "with freetype");
#else
add_assoc_bool(return_value, "FreeType Support", 0);
#endif
add_assoc_bool(return_value, "GIF Read Support", 1);
add_assoc_bool(return_value, "GIF Create Support", 1);
#ifdef HAVE_GD_JPG
add_assoc_bool(return_value, "JPEG Support", 1);
#else
add_assoc_bool(return_value, "JPEG Support", 0);
#endif
#ifdef HAVE_GD_PNG
add_assoc_bool(return_value, "PNG Support", 1);
#else
add_assoc_bool(return_value, "PNG Support", 0);
#endif
add_assoc_bool(return_value, "WBMP Support", 1);
#ifdef HAVE_GD_XPM
add_assoc_bool(return_value, "XPM Support", 1);
#else
add_assoc_bool(return_value, "XPM Support", 0);
#endif
add_assoc_bool(return_value, "XBM Support", 1);
#ifdef HAVE_GD_WEBP
add_assoc_bool(return_value, "WebP Support", 1);
#else
add_assoc_bool(return_value, "WebP Support", 0);
#endif
#ifdef HAVE_GD_BMP
add_assoc_bool(return_value, "BMP Support", 1);
#else
add_assoc_bool(return_value, "BMP Support", 0);
#endif
#ifdef HAVE_GD_AVIF
add_assoc_bool(return_value, "AVIF Support", 1);
#else
add_assoc_bool(return_value, "AVIF Support", 0);
#endif
#ifdef HAVE_GD_TGA
add_assoc_bool(return_value, "TGA Read Support", 1);
#else
add_assoc_bool(return_value, "TGA Read Support", 0);
#endif
#ifdef USE_GD_JISX0208
add_assoc_bool(return_value, "JIS-mapped Japanese Font Support", 1);
#else
add_assoc_bool(return_value, "JIS-mapped Japanese Font Support", 0);
#endif
)

@nielsdos
Copy link
Member

Ah and checking for imagegd2 (or similar) won't suffice because it seems they are not surrounded by ifdefs...

@cmb69
Copy link
Member

cmb69 commented Jul 17, 2024

See #12019. I think we need to do that for gd/gd2, too.

@orlitzky
Copy link
Contributor Author

Support for the legacy "gd" image format was removed from gd-2.3.3 upstream:
https://github.com/libgd/libgd/blob/master/CHANGELOG.md#233---2021-09-12

The changelog says: "support for the legacy/testing gd image formats is disabled by default". It is perfectly possible to still build libgd 2.3.3 with support for these formats (otherwise that would have been a serious BC break). ext/gd/tests/bug41442.phpt, for instance, wouldn't even need to have been touched, since the test has already been skipped, if imagegd2() is not available.

Sorry! I did this before #12019 and I didn't know of a better way to detect the supported formats at the time. It looks like now we just have to add them to the list of gdImageCreateFromFoo checks.

@cmb69
Copy link
Member

cmb69 commented Jul 18, 2024

I've started PR #15006.

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.

4 participants