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

Add option to enable large SVGs #201

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from
Open

Conversation

kirtangajjar
Copy link
Member

@kirtangajjar kirtangajjar commented May 19, 2024

Description of the Change

PR adds support of large SVG files to the plugin. It also adds a setting for it to let the users toggle this behaviour.

How to test the Change

Verify GH action run on this PR and it works properly.

Changelog Entry

Added - Option to enable large SVG file(>10M) upload

Closes #178

Credits

Props @kirtangajjar

Checklist:

  • I agree to follow this project's Code of Conduct.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests pass.

@github-actions github-actions bot added this to the 2.3.0 milestone May 19, 2024
@github-actions github-actions bot added the needs:feedback This requires feedback to determine next steps. label May 19, 2024
Copy link

@kirtangajjar thanks for the PR! Could you please fill out the PR template with description, changelog, and credits information so that we can properly review and merge this?

@github-actions github-actions bot added needs:code-review This requires code review. and removed needs:feedback This requires feedback to determine next steps. labels May 19, 2024
@jeffpaul jeffpaul requested a review from faisal-alvi May 20, 2024 13:06
Copy link
Member

@faisal-alvi faisal-alvi left a comment

Choose a reason for hiding this comment

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

@kirtangajjar Thanks for the PR. I tested and found the setting works incorrectly.

When "Allow large SVG files" is disabled, large files are actually uploading! I tested with this Phisical_Map_of_the_World_-_January_2015.svg (13.5 MB)

And, when "Allow large SVG files" is enabled, it throws an error and the upload fails with a note!

image

Can you please check what is causing this? you can use the same SVG I tested with

@kirtangajjar
Copy link
Member Author

@faisal-alvi Thanks for the feedback appreciate it. Even I found that many time we can upload larger files without that flag, but for some files we just are not able to upload them without that flag. i.e. This portrait image won't work without that flag. I found this image in this issue discussion. Many big files would work without the flag. I wasn't able to find any image which causes issue with the toggle on so I kept it on.

https://github.com/10up/safe-svg/assets/8456197/6805e259-26b5-45d1-af71-cbdc7135740c

@faisal-alvi
Copy link
Member

I wasn't able to find any image which causes issue with the toggle on so I kept it on.

@kirtangajjar have tried with the https://commons.wikimedia.org/wiki/Category:Large_SVG_files#/media/File:Political_map_of_the_World_(January_2015).svg file? I can reproduce the issue with this SVG file. The issue (as mentioned above) is that I can NOT upload the SVG when "Allow large SVG files" is enabled !? but I can, when it is disabled! I'm ready to connect 1:1 to test and discuss this in detail.

@kirtangajjar
Copy link
Member Author

@faisal-alvi That's really weird. I downloaded the exact same Image and it was working fine for me. Let's connect on it on a week where I have some OSP time.

@faisal-alvi
Copy link
Member

Sure @kirtangajjar! Ping me once you are available for a call.

@jeffpaul jeffpaul modified the milestones: 2.2.5, 2.3.0 Jun 27, 2024
Copy link
Member

@faisal-alvi faisal-alvi left a comment

Choose a reason for hiding this comment

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

@kirtangajjar we discussed over a call that you'll add a custom check to prevent SVG files larger than 10MB. The setAllowHugeFiles() function produces inconsistent results, sometimes allowing files over 10MB when it shouldn't and vice versa.

@github-actions github-actions bot added the needs:refresh This requires a refreshed PR to resolve. label Sep 2, 2024
@github-actions github-actions bot removed the needs:refresh This requires a refreshed PR to resolve. label Sep 7, 2024
Copy link
Member

@faisal-alvi faisal-alvi left a comment

Choose a reason for hiding this comment

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

@kirtangajjar is this ready for review?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs:code-review This requires code review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SVGs have a 10mb size Limit
4 participants