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

Update IcoParser.php to detect BMP files #18

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

devenbj
Copy link

@devenbj devenbj commented May 26, 2022

With newer versions of PHP, the imagecreatefromstring parses BMP files as well as PNG. The same function being used for PNG processing now works on BMP. I added detection code for BMP to the isSupportedBinaryString() and parse() functions PNG detection, and renamed the function and variables to reflect that it may not just be PNG files anymore.

I verified this on the php.net website, which uses a BMP as it's favicon.

For my implementation of the library, I am using my code to download the ico. I send it to icofileloader as a string.

This should address Issue #16

Updated to detect BMP icons
@lordelph
Copy link
Owner

This library is really intended to focus purely on .ico files, and not for general purpose image formats like bmp.

The PR itself causes test failures and would need further work before it was capable of loading a bmp image without error.

That said, the library does support loading icon files which are infact just plain png files, so it could be more thoroughly extended to support bmp if they are being used in the wild. You mention the php.net uses a bmp, but as far as I can tell, it's favicon.ico is a regular ico file. Have you got another example?

Issue 16 is more about how this library handles bmp format images held within ico files. At present, the library renders these pixel by pixel by parsing the BMP image data. The intention was to replace this with native BMP rendering as it became available in 7.2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants