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

Enable WebP images in WooCommerce #37307

Merged
merged 7 commits into from Apr 6, 2023

Conversation

anastas10s-afk
Copy link
Contributor

@anastas10s-afk anastas10s-afk commented Mar 19, 2023

All Submissions:

Changes proposed in this Pull Request:

WebP files should be able to be imported in WooCommerce, as they've been supported in WordPress core since v5.8.

Adding this pull request, based on the following comments:

This is a first for me, hope all is in order. Cheers! 🙂
PS: I am aware that line 2609 could use alignment with the lines above it; can't see that I have an option to edit the code at this point in time, though.

Closes #28998.

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes, as applicable?
  • Have you created a changelog file for each project being changed, ie pnpm --filter=<project> changelog add?
  • Have you included testing instructions?

FOR PR REVIEWER ONLY:

  • I have reviewed that everything is sanitized/escaped appropriately for any SQL or XSS injection possibilities. I made sure Linting is not ignored or disabled.

WebP files should be able to be imported in WooCommerce, as they've been supported in WordPress core since v5.8.

Adding this pull request, based on the following comments:
- woocommerce#28998 (comment)
- woocommerce#28998 (comment)

This is a first for me, hope all is in order. Cheers! 🙂
@github-actions github-actions bot added plugin: woocommerce Issues related to the WooCommerce Core plugin. type: community contribution labels Mar 19, 2023
@woocommercebot woocommercebot requested review from a team and rrennick and removed request for a team March 19, 2023 22:02
Copy link
Contributor

@rrennick rrennick left a comment

Choose a reason for hiding this comment

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

@anastas10s-afk Thanks for submitting the PR. See my comments for the needed changes.

PS: I am aware that line 2609 could use alignment with the lines above it; can't see that I have an option to edit the code at this point in time, though.

A pull request isn't restricted to a single commit. If you notice you have missed something then you can fix it in your PR branch and push it to Github. Github will automatically update your PR.

The PR template has Closes # . in it. The reason we have that in the template is that Github will automatically close the issue if the PR description has Closes #ISSUENUMBER. Can you add that to your description.

Lastly, our process requires a changelog entry. You can create one with pnpm --filter=woocommerce changelog add. If you don't have pnpm running in your dev environment, let me know and I can add the changelog entry for you.

return $mime_to_ext;
}

function wc_rest_allowed_image_mime_types() {
Copy link
Contributor

Choose a reason for hiding this comment

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

This function already exists: https://github.com/woocommerce/woocommerce/blob/trunk/plugins/woocommerce/includes/wc-rest-functions.php#L47:L59

Adding a second definition will cause a fatal error. You need to edit the original function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I understand, I should go ahead with removing the whole function that I added (starting at line 2599), at wc-core-functions.php, and add 'webp' => 'image/webp', in line 57 at wc-rest-functions.php.

Correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went ahead with doing so.

*
* WebP files should be able to be imported in WooCommerce, as they've been supported in WordPress core since v5.8.
*/
add_filter( 'woocommerce_rest_allowed_image_mime_types', 'more_mimes_to_exts' );
Copy link
Contributor

Choose a reason for hiding this comment

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

This filter isn't necessary if you are already adding webp to the array being passed to the filter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to clarify, it is suggested to remove line 2592. Correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went ahead with doing so.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just to clarify, it is suggested to remove line 2592.

There is no need for the more_mimes_to_exts function as it was being called by the filter.

@anastas10s-afk
Copy link
Contributor Author

anastas10s-afk commented Mar 21, 2023

A pull request isn't restricted to a single commit. If you notice you have missed something then you can fix it in your PR branch and push it to Github. Github will automatically update your PR.

TIL, thank you for this @rrennick.

The PR template has Closes # . in it. The reason we have that in the template is that Github will automatically close the issue if the PR description has Closes #ISSUENUMBER. Can you add that to your description.

Happily! Just to clarify, that would be Closes #28998 or Closes #37307, though?
Went ahead with Closes #28998.

Lastly, our process requires a changelog entry. You can create one with pnpm --filter=woocommerce changelog add. If you don't have pnpm running in your dev environment, let me know and I can add the changelog entry for you.

Kindly go ahead with adding the changelog entry for me, please. I do not have pnpm running in your dev environment. Thank you!

Removed, previously added, unnecessary filter, and already existing function in wc-rest-functions.php.
@rrennick
Copy link
Contributor

Closes #37307

@anastas10s-afk Merging a PR closes it. The Closes # automation only works with issues. Once you have removed the unnecessary function this will be good to go.

@anastas10s-afk
Copy link
Contributor Author

anastas10s-afk commented Mar 22, 2023

Closes #37307

@anastas10s-afk Merging a PR closes it. The Closes # automation only works with issues.

Noted. I went ahead with editing it to Closes #37307. now, thank you!

Once you have removed the unnecessary function this will be good to go.

@rrennick, as I understand the f2cd586 and bd2de5f commits are on point with fixing what is needed, and it is good to go? If otherwise, forgive me, could you reiterate on what would be needed from my end instead, please. Cheers!

@rrennick
Copy link
Contributor

Noted. I went ahead with editing it to Closes #37307. now, thank you!

@anastas10s-afk Sorry if I wasn't clear. Adding Closes #37307 has no effect because this PR will be closed automatically when it is merged. Closes #28998 needs to be in the description for Github to close the issue when the PR is merged.

There is no need for the more_mimes_to_exts function as it was being called by the filter.

This needs to be addressed before the PR can be merged.

@anastas10s-afk
Copy link
Contributor Author

anastas10s-afk commented Mar 23, 2023

There is no need for the more_mimes_to_exts function as it was being called by the filter.

This needs to be addressed before the PR can be merged.

I believe everything is set now, for the PR to be merged @rrennick.

To summarize this PR, I went ahead with removing all additions I previously made in /woocommerce/includes/wc-core-functions.php, at this commit, and kept only the one at /woocommerce/includes/wc-rest-functions.php - with this commit.

Cheers!

@rapsli
Copy link

rapsli commented Apr 4, 2023

any timeline on when this is live?

@rrennick
Copy link
Contributor

rrennick commented Apr 5, 2023

@anastas10s-afk Could you merge the latest trunk into this branch? We have some failing tests that were fixed in trunk.

@anastas10s-afk
Copy link
Contributor Author

@anastas10s-afk Could you merge the latest trunk into this branch? We have some failing tests that were fixed in trunk.

Happily ...if only I knew how to do so @rrennick. I cannot find my way around doing that merge, either the web interface or the desktop app, at GitHub. First-timer, my apologies.

@anastas10s-afk
Copy link
Contributor Author

@rrennick any pointers would be very much appreciated, if possible.

Screenshot 2023-04-06 at 9 31 47 PM

@codecov
Copy link

codecov bot commented Apr 6, 2023

Codecov Report

Merging #37307 (b0f042e) into trunk (817458a) will decrease coverage by 0.0%.
The diff coverage is 100.0%.

❗ Current head b0f042e differs from pull request most recent head 0d93e92. Consider uploading reports for the commit 0d93e92 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             trunk   #37307     +/-   ##
==========================================
- Coverage     51.7%    51.6%   -0.0%     
  Complexity   17257    17257             
==========================================
  Files          429      429             
  Lines        79765    79828     +63     
==========================================
+ Hits         41200    41202      +2     
- Misses       38565    38626     +61     
Impacted Files Coverage Δ
plugins/woocommerce/includes/wc-rest-functions.php 89.0% <100.0%> (+0.1%) ⬆️

... and 3 files with indirect coverage changes

@anastas10s-afk
Copy link
Contributor Author

@anastas10s-afk Could you merge the latest trunk into this branch? We have some failing tests that were fixed in trunk.

Quick update on this, @rrennick. From what I gather, I was able to do so?

Screenshot 2023-04-06 at 9 39 41 PM

@rrennick
Copy link
Contributor

rrennick commented Apr 6, 2023

any pointers would be very much appreciated, if possible.

@anastas10s-afk I did this for you earlier today. In the future the way you would do it is update trunk in your fork to the latest trunk in this repo, merge trunk into your branch, then push to your origin.

@rrennick rrennick merged commit ac06ebd into woocommerce:trunk Apr 6, 2023
16 of 18 checks passed
@github-actions github-actions bot added this to the 7.7.0 milestone Apr 6, 2023
@anastas10s-afk
Copy link
Contributor Author

@anastas10s-afk I did this for you earlier today. In the future the way you would do it is update trunk in your fork to the latest trunk in this repo, merge trunk into your branch, then push to your origin.

Much appreciated, truly. A huge learning opportunity for me. Have a great one, @rrennick!

@anastas10s-afk anastas10s-afk deleted the woocommerce-webp-support branch April 7, 2023 07:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plugin: woocommerce Issues related to the WooCommerce Core plugin. type: community contribution
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Webp Images not allowed even when allowd in Wordpress
3 participants