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

Allow cloning of GdImage from gd 2.2.3 #10241

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Conversation

blar
Copy link
Contributor

@blar blar commented Jan 6, 2023

From version 2.2.3 gd supports cloning https://libgd.github.io/manuals/2.2.3/files/gd-c.html#gdImageClone

The bundled version of gd is at version 2.0.35 so cloning is only supported with --with-external-gd

Copy link
Member

@iluuu1994 iluuu1994 left a comment

Choose a reason for hiding this comment

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

LGTM, but maybe @cmb69 wants to have a look?

ext/gd/tests/gdimage_cloning.phpt Outdated Show resolved Hide resolved
Copy link
Contributor

@cmb69 cmb69 left a comment

Choose a reason for hiding this comment

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

Thank you for the PR!

I fully agree that it makes sense to expose gdImageClone() to userland, but not 100% sure about whether via clone or a new function (imageclone()). Since we're having objects now, the former probably makes most sense.

Regarding the libgd version requirements: there are several known bugs in that function, and some have not even been fixed yet (albeit quite serious). Maybe we should require a newer version right away? On the other hand, bugs in external libraries are not uncommon, so we can stick with libgd ≥ 2.2.

The bundled version of gd is at version 2.0.35 so cloning is only supported with --with-external-gd

ext/gd is lying. :) There have been several updates (bugfixes and features), but the bundled gd never conformed to any particular libgd version. We might update these version numbers, although the long term goal is still to unbundle libgd (what is still blocked by libgd/libgd#335).

Anyhow, we could port gdImageClone() from libgd right away, so more users could use the new feature. This could alternatively be done via a separate follow-up PR.

ext/gd/tests/gdimage_cloning.phpt Outdated Show resolved Hide resolved
ext/gd/tests/gdimage_cloning.phpt Show resolved Hide resolved
ext/gd/tests/gdimage_cloning.phpt Outdated Show resolved Hide resolved
ext/gd/tests/gdimage_prevent_cloning.phpt Outdated Show resolved Hide resolved
@blar
Copy link
Contributor Author

blar commented Jan 6, 2023

I fully agree that it makes sense to expose gdImageClone() to userland, but not 100% sure about whether via clone or a new function (imageclone()). Since we're having objects now, the former probably makes most sense.

The function imageclone() only make sense if you want to backport it to a version of php before 8.0.0, which don't had the GdImage class. Additionally developers may question why every other (clonable) class can be cloned with clone, but GdImage requires imageclone().

@cmb69
Copy link
Contributor

cmb69 commented Jan 6, 2023

Additionally developers may question why every other (clonable) class can be cloned with clone, but GdImage requires imageclone().

I agree (although devs also may wonder why there are no methods for this class). :)

@TimWolla
Copy link
Member

TimWolla commented Jan 6, 2023

With #10225, I agree that the clone operator is the right choice.

ext/gd/gd.c Outdated Show resolved Hide resolved
@blar blar requested a review from cmb69 February 6, 2023 16:48
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.

None yet

6 participants