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

Can't upload ico favicons #73

Open
SychO9 opened this issue Jul 1, 2021 · 0 comments
Open

Can't upload ico favicons #73

SychO9 opened this issue Jul 1, 2021 · 0 comments

Comments

@SychO9
Copy link
Member

SychO9 commented Jul 1, 2021

Bug Report

Current Behavior
ico favicons cannot no longer be uploaded.

Steps to Reproduce

  1. Go to Dashboard Apearence page.
  2. Upload ico favicon.
  3. Notice it errors out with a 500.

Environment

  • Flarum version: 1.0.3 (but introduced in beta 15)

Possible Solution
An abstract UploadImage controller class was introduced in beta 15, the makeImage method is expected to return a Intervention\Image\Image object, which is not the case for ico files where we return the file stream, this is because the GD extension used by intervention, does not support ico files.

https://github.com/flarum/core/blob/a077ae9ca36a8ad0adc25042118ba2407feaa5fe/src/Api/Controller/UploadFaviconController.php#L29-L42

We could remove the explicit typehint on the makeImage method, but that would break extensions, so the best thing to do is to throw a validation error on ico images in 1.x (since there is no way to fix this without making breaking changes) and we can fix the issue for 2.0

Additional Context
Relevant PR: flarum/framework#2477

@SychO9 SychO9 added the type/bug label Jul 1, 2021
@SychO9 SychO9 self-assigned this Jul 1, 2021
@askvortsov1 askvortsov1 transferred this issue from flarum/framework Mar 10, 2022
@askvortsov1 askvortsov1 transferred this issue from flarum/issue-archive Mar 10, 2022
@askvortsov1 askvortsov1 transferred this issue from flarum/framework Mar 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant