Skip to content

Conversation

nikic
Copy link
Member

@nikic nikic commented Oct 9, 2020

I've also changed a couple of boolean int parameters to bool. While doing that I noticed that imagerotate() has an $ignore_transport argument that is ... ignored :) What's up with that?

@kocsismate
Copy link
Member

Soo we're almost done!

While doing that I noticed that imagerotate() has an $ignore_transport argument that is ... ignored :) What's up with that?

ext/gd has a nice track record for this :) I can remember about 1-2 similar cases :D

Copy link
Member

@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.

Thanks! Looks good to me.

Regarding the ignoretransparent parameter of imagerotate(): this has been added as "bug" fix, but basically removed when the improved rotate functions became available which don't support this "feature" any longer, because they enforce conversion to true color, where the transparent color is not relevant. There is a comment in the code which points out that palette color support could be added at a later time, but I have doubts that will ever happen; I'd rather get rid of internal palette image support at all (IMO the tighter memory footprint of palette images is not worth the trouble for userland developers as well as libgd maintainers).

#endif

function imagerotate(GdImage $im, float $angle, int $bgdcolor, int $ignoretransparent = 0): GdImage|false {}
function imagerotate(GdImage $image, float $angle, int $bgdcolor, int $ignoretransparent = 0): GdImage|false {}
Copy link
Member

Choose a reason for hiding this comment

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

Maybe just $bgcolor instead of $bgdcolor?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hrm, it looks like I missed to commit part of my changes :/ This was supposed to be $background_color/$foreground_color. Though if those are too long, I'm also fine with $bg_color and $fg_color.

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with $background_color/$foreground_color.

@nikic
Copy link
Member Author

nikic commented Oct 9, 2020

Regarding the ignoretransparent parameter of imagerotate(): this has been added as "bug" fix, but basically removed when the improved rotate functions became available which don't support this "feature" any longer, because they enforce conversion to true color, where the transparent color is not relevant. There is a comment in the code which points out that palette color support could be added at a later time, but I have doubts that will ever happen; I'd rather get rid of internal palette image support at all (IMO the tighter memory footprint of palette images is not worth the trouble for userland developers as well as libgd maintainers).

Any thoughts on what we should do with the parameter? Should we just keep it as is (ignored), or should we drop it?

@cmb69
Copy link
Member

cmb69 commented Oct 9, 2020

Any thoughts on what we should do with the parameter? Should we just keep it as is (ignored), or should we drop it?

Hmm, not sure if we can still drop it for PHP 8.0. If not probably better go through deprecation for 8.1.

Copy link
Member

@kocsismate kocsismate left a comment

Choose a reason for hiding this comment

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

I'm good with the changes!

@php-pulls php-pulls closed this in 14a26db Oct 9, 2020
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.

3 participants