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

OMP error when uploading cover image in WebP or SVG format #7400

Closed
NateWr opened this issue Oct 26, 2021 · 7 comments
Closed

OMP error when uploading cover image in WebP or SVG format #7400

NateWr opened this issue Oct 26, 2021 · 7 comments
Assignees
Labels
Bug:1:Low A bug that does not have a severe consequence or affects a small number of users. Try Me This issue might be good for a new contributor. Can you help us?
Milestone

Comments

@NateWr
Copy link
Contributor

NateWr commented Oct 26, 2021

Describe the bug
When uploading a cover image in WebP or SVG format, OMP tries to create a thumbnail but cant, so it throws a fatal error.

Can not build thumbnail because the file was not found or the file extension was not recognized.

It looks like the code at PublicationService::makeThumbnail() (stable-3_3_0) supports jpg, png and gif. It can also support webp with imagecreatefromwebp. However, SVG is trickier. There's no such thing as a "thumbnail" for a SVG. Maybe we can just copy the file to the thumbnail location for now.

To Reproduce
Steps to reproduce the behavior:

  1. In OMP, go to a submission's Publication tab, open Catalog Entry, and try to upload a .webp or .svg file.
  2. See error in log

What application are you using?
OMP (maybe 3.3.0-x)

Additional information
See screenshots and full error in forum post.

@NateWr NateWr added the Bug:1:Low A bug that does not have a severe consequence or affects a small number of users. label Oct 26, 2021
@NateWr NateWr added this to the 3.3.0-9 milestone Oct 26, 2021
@NateWr NateWr added the Try Me This issue might be good for a new contributor. Can you help us? label Oct 26, 2021
@withanage withanage self-assigned this Nov 21, 2021
@withanage
Copy link
Member

withanage commented Nov 21, 2021

@NateWr I have self-assigned this one and have looked into webp issue.

PRS for webp support

However, SVG is trickier. There's no such thing as a "thumbnail" for a SVG. Maybe we can just copy the file to the thumbnail location for now.

Should I may be look into copying the svg functionality too?

FYI @asmecher

withanage added a commit to withanage/pkp-lib that referenced this issue Nov 21, 2021
@NateWr
Copy link
Contributor Author

NateWr commented Nov 22, 2021

@withanage yes I think that will be easiest. It will result in some file duplication, but the alternative will require some tricky handling of the thumbnail filename. Right now the application assumes the thumbnail is a different file with a specific filename.

@withanage
Copy link
Member

withanage commented Nov 25, 2021

@NateWr

I added the svg copying to the thumbnail creation function. Seemed to me adding to the addPublicaiton and editPublication would mean more code-duplication. But logically , if it seems better to handle the copying there also regarding the future vector-graphic forms , please let me know.

Updated PRS

@NateWr
Copy link
Contributor Author

NateWr commented Nov 25, 2021

This looks great, and very clean. Thanks @withanage! In the pkp-lib pull request, you have added support for .webp. Should .svg be added here too?

Finally, can you add the submodule commit to OMP so the tests will run against the correct copy of pkp-lib?

@withanage
Copy link
Member

withanage commented Nov 25, 2021

Thanks @NateWr

the pkp-lib pull request, you have added support for .webp. Should .svg be added here too?

Sorry, I missed that, thanks. I have updated the PR with the additions to image file validation for svg. In FileManager svg is already present.

Finally, can you add the submodule commit to OMP so the tests will run against the correct copy of pkp-lib?

I added the submodule update and triggered the tests.

NateWr added a commit that referenced this issue Nov 25, 2021
#7400 Upload support for image format webp and svg
@NateWr
Copy link
Contributor Author

NateWr commented Nov 25, 2021

Thanks @withanage! I've merged those PRs and applied the changes to main at 29118ca and pkp/omp@034c809. Can you just look at those two commits quickly to double-check my work?

@NateWr NateWr closed this as completed Nov 25, 2021
@withanage
Copy link
Member

Thanks @NateWr for forward-porting to main.
I have doubled-checked both those commits 29118ca and pkp/omp@034c809 . They both look good !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug:1:Low A bug that does not have a severe consequence or affects a small number of users. Try Me This issue might be good for a new contributor. Can you help us?
Projects
None yet
Development

No branches or pull requests

2 participants