Feature/svgo support #79

gsarig · 2022-10-09T20:09:19Z

Description of the Change

Implements support for SVG optimization using the popular SVGO tool. For the scripts' structure and build, 10up-toolkit has been utilized. The Optimizer is active by default and will try to optimize any SVG file uploaded via the Media Library, using the default SVGO settings.

A filter safe_svg_svgo_param has been added so that developers can override these settings and pass their own.
Also, using the same filter like this: add_filter( 'safe_svg_svgo_params', '__return_false' ); will allow them to completely disable the Optimizer.

~~At the moment, the optimizer doesn't work if you upload an image directly (using "Upload" instead of the "Media Library" button), as it bypasses the media uploader.~~ Other than that, as long as the file is uploaded using the Media Library, it will get optimized. Also, it should work with both single and multiple images. With the first chance, I will resume my work to make it work with direct uploads as well.

Closes #68

How to test the Change

Run npm install and npm run build.
Upload one or more unoptimized SVG files using the Media Library. You can find unoptimized SVG files for testing purposes here.
Confirm that the uploaded file(s) have been optimized. This can be checked by either comparing their file size or by adding the files in a text editor and comparing their contents (optimized files should have considerably less content).

Known issue: Given that SVGO is a JS tool, using it with WordPress had its challenges. To make it work, a hook to wp.Uploader has been made, to identify when a file gets uploaded, optimize it, and then make an AJAX call to update the file with PHP. ~~Therefore, at the moment it doesn't work if you upload an image directly, bypassing the media uploader. More specifically, if you hit the "Upload" button from the image below:~~

Using the "Media Library" option, on the other hand, should work with either single or multiple SVG. It should also work with drag and drop and even if you drop mixed image types (for example SVGs together with JPEGs, PNGs etc) at the same time.

Update: Support for optimizing on direct uploads has been added on this commit, and images uploaded using the direct upload button will get optimized when the post is saved.

Changelog Entry

Added - SVG optimization during upload via SVGO

Credits

Props @gsarig

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.

jeffpaul · 2022-10-18T17:58:13Z

@Sidsector9 @darylldoyle looking for review from you both on this, thanks!

Sidsector9

Thank you very much for working on this PR.

Tested by uploading a 1KB SVG using the Media Uploader. The size got reduced to 411 bytes.
Tested the filter add_filter( 'safe_svg_svgo_params', '__return_false' ); to disable optimisation, works as ecpected.

This is the image I used:

Sidsector9 · 2022-10-19T06:57:57Z

@jeffpaul I am wondering if we can extend this to a separate feature to support a batch optimiser, which will optimise already uploaded SVG images, what do you think?

Direct upload doesn't work

Sidsector9

Thank you very much for working on this PR.

✅ Tested by uploading a 1KB SVG using the Media Uploader. The size got reduced to 411 bytes.
✅ Tested the filter add_filter( 'safe_svg_svgo_params', '__return_false' ); to disable optimisation, works as excpected.
❌ Direct upload using core/image block did not optimise the SVG

This is the image I used:

Please watch the video below:

SVGO.mov

gsarig · 2022-10-19T07:38:09Z

Thank you very much for working on this PR.

✅ Tested by uploading a 1KB SVG using the Media Uploader. The size got reduced to 411 bytes. ✅ Tested the filter add_filter( 'safe_svg_svgo_params', '__return_false' ); to disable optimisation, works as excpected. ❌ Direct upload using core/image block did not optimise the SVG

This is the image I used:

Please watch the video below:

SVGO.mov

Hi @Sidsector9 Thanks for testing this. This is something that I should have mentioned in the PR (I've updated the description): the image will get optimized on post save. So, if you click to update the post, the image should get optimized.

Sidsector9

Thanks for the additional details @gsarig. LGTM, approving 👍

gsarig · 2022-10-19T08:14:55Z

@jeffpaul I am wondering if we can extend this to a separate feature to support a batch optimiser, which will optimise already uploaded SVG images, what do you think?

@Sidsector9 @jeffpaul My thought here was to allow it to optimize existing SVGs when someone triggers a batch regenerate of images, with WP-CLI or a third-party plugin. That way we wouldn't have to build a settings page for that, or a specific mechanism from scratch and the implementation should be quicker.
The challenge with the optimizer is that being a JS tool, it cannot utilize the standard PHP hooks so it would take some testing/investigation to see how this could best work.

Sidsector9 · 2022-10-20T07:05:05Z

@gsarig yeah a feature like that would be a good addition to the plugin's capabilities. We can open a separate issue to investigate how best to approach this.

jeffpaul · 2022-10-20T13:29:49Z

@vikrampm1 can you please open an issue as noted by Sid and Giorgos above?

darylldoyle · 2022-10-21T13:06:57Z

assets/js/admin/admin.js

+                    const svgUrl = changedBlock?.attributes?.url;
+
+                    // Run only if the image is an SVG that has not been optimized yet.
+                    if (!svgUrl || !svgUrl.endsWith('.svg')) {


thought (non-blocking): This will only pick up files with the .svg extension. What about .svgz or SVGs that are uploaded without the SVG extension?

Do we have access to the mime type at this point to check that instead?

@darylldoyle We don't (that's the reason I made the check based on the filename). Here's an example of what the changedBlock returns:

One approach that I can think of is to remove that particular check and perform it inside the fetch, where the file's content-type becomes available. In that case, though, the fetch would have to run on every file, while now we only run it on .svg files. Perhaps we could mitigate that by narrowing it down to specific extensions (e.g. svg* for anything that ends in svg-something) and files with no extension at all. That way, we wouldn't bother running it on jpgs, pngs etc.

What do you think?

darylldoyle · 2022-10-21T13:21:37Z

assets/js/admin/admin.js

+            for (const changedBlock of changes.blocks) {
+
+                // Run only if this is a new core image block.
+                if ('core/image' === changedBlock.name && !changedBlock?.originalContent) {


thought (non-blocking): This only works for core/image blocks. What about core/gallery, core/media-text etc.

@darylldoyle I've pushed a new commit, which adds support for core/media-test and for nested blocks containing images (which covers the core/gallery scenario).
Can you please take a look?
Thanks!

darylldoyle · 2022-10-21T13:27:00Z

assets/js/admin/admin.js

+    subscribe(() => {
+        if (isSavingPost()) {


thought (non-blocking): The handling of this on save concerns me a little. We're making 2 additional requests for each image on the page. That could add up quickly and really bog down the save process of the block editor.

Have we looked other options, such as something like https://github.com/spatie/image-optimizer which may allow us to do the optimisation from PHP at upload time.

@darylldoyle I agree with your arguments, and this is something that concerned me as well before I ended up with the specific approach. When testing the image-optimizer, though, I came across the following issues, which made me avoid it:

It uses SGVO 1, which is the older version of SVGO, and based on that comment, it seems to have a vulnerable dependency.

When I tried to test it, I got a fatal error, as one of the tool's dependencies seem to require PHP 8.1.0. (The error was Your Composer dependencies require a PHP version ">= 8.1.0"..

I do agree, that a PHP-based approach would be ideal, and much simpler to implement. I didn't find a reliable tool to use instead, though.

jeffpaul · 2022-10-24T16:49:11Z

@gsarig some input from Daryll above, let me know if you hope to be able to react to those or if I should pull someone additional to help finish this off?

gsarig · 2022-10-24T16:57:46Z

@gsarig some input from Daryll above, let me know if you hope to be able to react to those or if I should pull someone additional to help finish this off?

Hi @jeffpaul
I saw Daryll's input - I will finish this, probably this weekend.

jeffpaul · 2022-11-10T00:08:57Z

@darylldoyle @Sidsector9 looking to either for a review on the updates here

peterwilsoncc

I've added a few notes inline.

While manually testing, I'm not seeing the changes been saved while logged in as an administrator.

I uploaded an SVG with a bunch of needless <g> tags, the optimiser picks it up client side and sends the network request, but the file isn't been updated.

includes/optimizer.php

peterwilsoncc · 2023-03-26T23:44:10Z

assets/js/admin/admin.js

+import {select, subscribe} from '@wordpress/data';
+
+(function () {
+    const ajaxUrl = new URL(safeSvgParams.ajaxUrl); // eslint-disable-line no-undef


Rather than disable the rule each time, I'd suggest defining the global in a comment at the start of the file

/* global safeSvgParams */

@peterwilsoncc Done

peterwilsoncc · 2023-03-26T23:48:21Z

assets/js/admin/admin.js

+            form.addEventListener('submit', function (event) {
+                const fileInput = document.querySelector('#async-upload');
+                if (fileInput.files[0].type === 'image/svg+xml' || fileInput.files[0].name.endsWith('.svg')) {
+                    document.cookie = safeSvgCookie + '=1;' + safeSvgCookieAttr;


WordPress includes wpCookies with a bunch of cookie helper functions, it's enqueued on most pages in the admin so you should be able to use that.

Thanks for that tip @peterwilsoncc I've updated the code to use wpCookies. I couldn't find much documentation about wpCookies so I went to the core to see how it works. Can you please take a look to check if the implementation is good?

gsarig · 2023-05-21T13:24:15Z

I've added a few notes inline.

While manually testing, I'm not seeing the changes been saved while logged in as an administrator.

I uploaded an SVG with a bunch of needless <g> tags, the optimiser picks it up client side and sends the network request, but the file isn't been updated.

Thanks for the feedback @peterwilsoncc
I've pushed fixes based on your feedback, and I've also merged develop into the branch, to solve some conflicts.

I've been testing it with unoptimized SVGs taken from here, as well as with the custom.svg from the Cypress tests, and on both cases I could see a difference in size between the unoptimized and the optimized file.

One additional thing that I noticed was that Cypress reported a problem with a failed test: Plugin should allow modify allowed tags and attributes:. After some digging to understand why it happened, I realized that it sort of makes sense: When the optimizer runs, it cleans up any unknown tags and attributes that the devs might have passed using the svg_allowed_attributes and svg_allowed_tags filters.

I searched in the SVGO library for any option to allow specific tags and attributes, but I couldn't find something. So, what I ended up doing was adding an additional check (see here) to disable the optimizer if a dev has added allowed tags or attributes.

Please let me know what you think about that and if you have any other suggestions.

peterwilsoncc

I've added a few more notes inline, including why the optimization was failing when I tested earlier.

In the JS, it should be possible to use the attachments ID and pass that to your ajax request in place of the URL. That will make the admin-ajax function a little more straight forward.

peterwilsoncc · 2023-05-22T05:05:22Z

includes/optimizer.php

+			if ( empty( $parsed_url['path'] ) ) {
+				return false;
+			}
+			$file = ABSPATH . ltrim( $parsed_url['path'], '/' );


I figured out why I wasn't getting the optimised version of the image.

I was testing on a site running WP in sub-directory mode with the directory structure:

/ #root directory /content #content directory /wp # WordPress install.

Instead of path/to/content/.../file.svg the $file is calculated as path/to/wp/content/.../file.svg.

In the method above, I've suggested you use get_attached_file() which will remove the need for this method.

Thanks for that @peterwilsoncc I followed your suggestion and on my local tests it seems to be working.

peterwilsoncc · 2023-05-22T05:05:57Z

includes/optimizer.php

+		 */
+		public function optimize() {
+			$svg_url = filter_input( INPUT_GET, 'svg_url', FILTER_SANITIZE_URL );
+			if ( ! current_user_can( 'edit_posts', attachment_url_to_postid( $svg_url ) ) ) {


Meta capabilities (ie, for the specific post) use the singular.

Suggested change

if ( ! current_user_can( 'edit_posts', attachment_url_to_postid( $svg_url ) ) ) {

if ( ! current_user_can( 'edit_post', attachment_url_to_postid( $svg_url ) ) ) {

You are right, I missed that. This should be fixed now.

includes/optimizer.php

peterwilsoncc · 2023-05-22T05:14:10Z

includes/optimizer.php

+			);
+			$params = wp_json_encode(
+				[
+					'ajaxUrl'    => esc_url( admin_url( 'admin-ajax.php' ) ),


I'm unsure, but I a suspect this would be better as sanitize_url(), we're wanting to make sure the URL is safe -- json-encode makes sure it renders correctly for JS.

sanitize_url() has been deprecated, and WP suggests using esc_url_raw() instead, so that's what I changed it to.

peterwilsoncc · 2023-05-22T05:16:04Z

includes/optimizer.php

+			$params = wp_json_encode(
+				[
+					'ajaxUrl'    => esc_url( admin_url( 'admin-ajax.php' ) ),
+					'svgoParams' => wp_json_encode( $this->svgo_params() ),


I'm confused by this, why aren't you passing this as is to the outer wp_json_encode call -- then you won't need to call JSON.parse in the JavaScript file.

assets/js/admin/admin.js

gsarig · 2023-05-27T20:52:32Z

I've added a few more notes inline, including why the optimization was failing when I tested earlier.

In the JS, it should be possible to use the attachments ID and pass that to your ajax request in place of the URL. That will make the admin-ajax function a little more straight forward.

Thank you for the great feedback @peterwilsoncc
I pushed some fixes based on your suggestions.

peterwilsoncc

LGTM, thanks for your patience during the review.

I noticed the optimize ajax request was been made twice on each save when I uploaded directly in to the block editor but have not been able to figure out why. It appears the wp.data subscription runs twice but that doesn't seem like something we have control over.

I think this is good to merge but attach a gif of what I am seeing in case inspiration strikes you.

Out of date

darylldoyle · 2023-07-15T08:05:53Z

One additional thing that I noticed was that Cypress reported a problem with a failed test: Plugin should allow modify allowed tags and attributes:. After some digging to understand why it happened, I realized that it sort of makes sense: When the optimizer runs, it cleans up any unknown tags and attributes that the devs might have passed using the svg_allowed_attributes and svg_allowed_tags filters.

I'd love to know what sort of attributes we're seeing stripped here. This is something I was looking into with the previous Pro implementation of SVGO and I have a feeling there are ways around it using SVGO plugins

For example, this is the plugin that removes the unknowns. The knowns are stored in a collections file. Perhaps we could extend that with our custom properties

https://github.com/svg/svgo/blob/main/plugins/removeUnknownsAndDefaults.js

gsarig added 4 commits October 4, 2022 02:01

DRAFT: Setup scripts

f361bf6

Implements the SVGO optimizer.

1cc8d6d

Run the JS only if the uploaded file is an SVG.

9a3baff

Addressing PHPCS issues

a9e6fdd

gsarig mentioned this pull request Oct 9, 2022

SVGO Optimisation #68

Closed

1 task

jeffpaul assigned gsarig Oct 10, 2022

jeffpaul added this to the 2.1.0 milestone Oct 10, 2022

jeffpaul requested review from darylldoyle, a team and Sidsector9 and removed request for a team October 10, 2022 14:16

Add support for optimizing SVGs on direct upload.

bcc12c2

gsarig marked this pull request as ready for review October 16, 2022 20:03

Sidsector9 previously approved these changes Oct 19, 2022

View reviewed changes

Sidsector9 requested changes Oct 19, 2022

View reviewed changes

Sidsector9 previously approved these changes Oct 19, 2022

View reviewed changes

vikrampm1 mentioned this pull request Oct 20, 2022

Optimise already uploaded SVG images #81

Open

1 task

darylldoyle reviewed Oct 21, 2022

View reviewed changes

Adds support for more core blocks

8142d6e

gsarig dismissed Sidsector9’s stale review via 8142d6e October 30, 2022 22:19

dkotter added this to the 2.2.0 milestone Mar 22, 2023

gsarig added 2 commits March 25, 2023 00:45

Merge branch 'develop' into feature/svgo-support

609829a

Improves the optimization logic when on the "Upload New Media" screen

e12ebd1

gsarig force-pushed the feature/svgo-support branch from dabd617 to e041baf Compare March 25, 2023 00:31

Updates composer.lock and package-lock.json files from develop

301de4d

gsarig force-pushed the feature/svgo-support branch from e041baf to 301de4d Compare March 25, 2023 00:36

Adds assets/js/admin/admin.js file to .distignore

37688c2

gsarig force-pushed the feature/svgo-support branch from c317cb0 to 37688c2 Compare March 26, 2023 20:18

peterwilsoncc reviewed Mar 27, 2023

View reviewed changes

Merge branch 'develop' into feature/svgo-support

5443373

gsarig force-pushed the feature/svgo-support branch from d6c933c to 5443373 Compare May 20, 2023 11:39

gsarig added 3 commits May 20, 2023 17:59

Addressing CR feedback

0c4c5d7

Restores composer.lock from develop

dce4baf

Fix phpcs errors

f0a8e1b

gsarig force-pushed the feature/svgo-support branch from c63a78b to be63c0b Compare May 20, 2023 22:58

Disables the optimizer if excluded tags or attributes are passed

5737372

gsarig force-pushed the feature/svgo-support branch from be63c0b to 5737372 Compare May 20, 2023 22:59

peterwilsoncc reviewed May 22, 2023

View reviewed changes

Addressing CR feedback

94316e2

gsarig force-pushed the feature/svgo-support branch from 2d71745 to 94316e2 Compare May 27, 2023 20:39

peterwilsoncc approved these changes May 29, 2023

View reviewed changes

peterwilsoncc merged commit d0b3ce6 into develop May 30, 2023

peterwilsoncc deleted the feature/svgo-support branch May 30, 2023 05:45

dkotter mentioned this pull request Jul 3, 2023

Release version 2.2.0 #132

Closed

19 tasks

dkotter mentioned this pull request Aug 16, 2023

Disable SVG optimization by default #145

Merged

4 tasks

dkotter mentioned this pull request Jan 18, 2024

When svg optimization is turned on, a javascript error can occur that breaks saving content #170

Closed

1 task

	if ( ! current_user_can( 'edit_posts', attachment_url_to_postid( $svg_url ) ) ) {
	if ( ! current_user_can( 'edit_post', attachment_url_to_postid( $svg_url ) ) ) {

Feature/svgo support #79

Feature/svgo support #79

Conversation

gsarig commented Oct 9, 2022 • edited Loading

Description of the Change

How to test the Change

Changelog Entry

Credits

Checklist:

jeffpaul commented Oct 18, 2022

Sidsector9 left a comment • edited Loading

Choose a reason for hiding this comment

Sidsector9 commented Oct 19, 2022

Sidsector9 left a comment

Choose a reason for hiding this comment

gsarig commented Oct 19, 2022

Sidsector9 left a comment

Choose a reason for hiding this comment

gsarig commented Oct 19, 2022 • edited Loading

Sidsector9 commented Oct 20, 2022

jeffpaul commented Oct 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeffpaul commented Oct 24, 2022

gsarig commented Oct 24, 2022

jeffpaul commented Nov 10, 2022

peterwilsoncc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gsarig commented May 21, 2023

peterwilsoncc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gsarig commented May 27, 2023

peterwilsoncc left a comment

Choose a reason for hiding this comment

darylldoyle commented Jul 15, 2023 • edited Loading

gsarig commented Oct 9, 2022 •

edited

Loading

Sidsector9 left a comment •

edited

Loading

gsarig commented Oct 19, 2022 •

edited

Loading

darylldoyle commented Jul 15, 2023 •

edited

Loading