Skip to content

Fix parameter numbers and missing alpha check for imagecolorset() #14477

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

Conversation

thg2k
Copy link
Contributor

@thg2k thg2k commented Jun 5, 2024

This PR addresses two issues:

  1. The parameter number reference is wrong
  2. The check for the alpha parameter was lost during the PHP 8.0 refactoring (commit 5076507) probably due to some copy-paste action.

I'm targeting master due to the additional check in the alpha parameter, as it is not a bug strictly speaking, I wouldn't like to break code that relies on this loose behavior on a patch release.

I'm available to retarget a lower branch or even split this into two PRs: one for fixing the parameter indexes (that is a bug), and one for adding the missing check (targeting master), just let me know.

The check for the alpha parameter existed in PHP 7.4 but was lost in PHP 8.0.

Fixes: 5076507
@thg2k thg2k force-pushed the pr/imagecolorset branch from 7f35ba6 to 0fb4db9 Compare June 5, 2024 07:16
Copy link
Member

@nielsdos nielsdos left a comment

Choose a reason for hiding this comment

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

Great catch!
I'll merge it into master and then pick up those parameter number fixes on lower branches, that will be easier than making 2 PRs.

@nielsdos nielsdos closed this in 44cbdb1 Jun 5, 2024
nielsdos pushed a commit that referenced this pull request Jun 5, 2024
This is the 8.2 & 8.3 version of GH-14477.
@nielsdos
Copy link
Member

nielsdos commented Jun 5, 2024

Merged manually.
For 8.2 & 8.3: da769be
For master: 44cbdb1

@thg2k thg2k deleted the pr/imagecolorset branch June 5, 2024 17:29
@thg2k
Copy link
Contributor Author

thg2k commented Jun 5, 2024

Wonderful, that's exactly how I would've done it, including the test with the NULL 😄

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.

2 participants