ext/gd: imageclone support.

[devnexen]

No description provided.

[thg2k]

I believe it would cause a memory leak if the cloned image has a tile or a brush. And likely an infinite recursion if the tile has the cloned image as its tile.. but that's something that should be addressed by libgd.

[thg2k]

Also, this overlaps with #10241. Personally I like the explicit imageclone better than the clone $im approach, especially because it's nicer to feature check with function_exists("imageclone") rather than try { clone $im } catch (Error ...).

[kocsismate]

LGTM but I'm not familiar with ext/gd... It would be nice to find someone who is familiar with it.

[cmb69]

It would be nice to find someone who is familiar with it.

Given that @devnexen is code owner of ext/gd and also maintainer of libgd, I would think this is not an issue. :)

Generally, I agree that we should support gdImageClone(), but I'm still not quite sure whether as function or via the clone operator. @thg2k has a good point regarding feature detection, though.

But regardless whether we take this approach or the one from #10241, we should port gdImageClone() into our bunded libgd.

[thg2k]

Apparently gdImageClone made it to the bundled libgd in #15640.

Now, what about the userland method? Why can't we have both imageclone and clone $im support?

Also I would still unset brushes and tiles from the cloned image to avoid memory leaks. But there are other issues with those anyway.

[devnexen]

Apparently gdImageClone made it to the bundled libgd in #15640.

Now, what about the userland method? Why can't we have both imageclone and clone $im support?

having both makes sense for me

[cmb69]

Now, what about the userland method? Why can't we have both imageclone and clone $im support?

What about having GdImage::clone() as well? ;)

Also I would still unset brushes and tiles from the cloned image to avoid memory leaks. But there are other issues with those anyway.

That should be reported to upstream, and fixed there first. In my opinion, fixing our bundled libgd should be a last resort.

[thg2k]

Now, what about the userland method? Why can't we have both imageclone and clone $im support?

What about having GdImage::clone() as well? ;)

I can sense the sarcasm, but yes why not adding all image functions as methods of the gd object? Similar to the mysqli approach. But that's off topic here. If this happens, i'd really like to see the imageclone function for consistency. Mixing oop and functional approach is just ugly.

How about we go with the function now and then we do a separate pr where we alias all the functions and add the clone as well?

Also I would still unset brushes and tiles from the cloned image to avoid memory leaks. But there are other issues with those anyway.

That should be reported to upstream, and fixed there first. In my opinion, fixing our bundled libgd should be a last resort.

I would, and also report other minor problems I found, but the project looks dead in the water.

[cmb69]

[…], but yes why not adding all image functions as methods of the gd object?

That is actually the long-term goal; changing resources to opaque objects was/is just a stop-gap measure to get of resources as soon as possilbe.

I would, and also report other minor problems I found, but the project looks dead in the water.

I know at least two libgd maintainers who are actively working on php-src; these might find some time to at least provide fixes against libgd (ideally just merging a PR).

[pierrejoye]

See #22532
ext/gd exposing it will be another PR on this PR is merged.