Enable end user opt-in to generate all sizes in fallback format #1689

b1ink0 · 2024-11-21T12:52:17Z

Summary

Relevant technical choices

This PR adds a checkbox users can check right below the "generate fallback images" checkbox (and it would only be enabled when "generate fallback images" checkbox is checked).

Settings Page Screenshots

Sample outputs for the below custom sizes

add_image_size( 'custom_size_1', 400, 400, true );
add_image_size( 'custom_size_2', 800, 800, true );
add_image_size( 'custom_size_3', 1200, 1200, true );

When the new option is off
When the new option is on

…lity to use added new settings

…izes option

…sizes

github-actions · 2024-11-21T13:01:53Z

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: b1ink0 <b1ink0@git.wordpress.org>
Co-authored-by: felixarntz <flixos90@git.wordpress.org>
Co-authored-by: mukeshpanchal27 <mukesh27@git.wordpress.org>
Co-authored-by: adamsilverstein <adamsilverstein@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

felixarntz

@b1ink0 A bit of feedback here, but overall this looks great!

felixarntz · 2024-11-21T18:15:34Z

plugins/webp-uploads/helper.php

+ *
+ * @return bool True if the option is enabled, false otherwise.
+ */
+function webp_uploads_is_fallback_all_sizes_enabled(): bool {


Maybe a better name would be something like webp_uploads_should_generate_all_fallback_sizes()?

felixarntz · 2024-11-21T18:20:33Z

plugins/webp-uploads/hooks.php

-		$allowed_sizes[ $size ] = ! empty( $size_details['provide_additional_mime_types'] );
+		if ( isset( $size_details['provide_additional_mime_types'] ) ) {
+			// Give priority to the 'provide_additional_mime_types' property.
+			$allowed_sizes[ $size ] = (bool) $size_details['provide_additional_mime_types'];
+		} else {
+			// If 'provide_additional_mime_types' is not set, use the fallback all sizes setting.
+			$allowed_sizes[ $size ] = webp_uploads_is_fallback_all_sizes_enabled();
+		}


Rather than doing this here, I think a more elegant solution would be to decouple the UI control and option from the main logic, by using the webp_uploads_image_sizes_with_additional_mime_type_support filter below. So I think this would be better separately implemented.

It would also allow the code to be a bit more efficient: We should only need to call webp_uploads_is_fallback_all_sizes_enabled() once, not for every size available. If false we can immediately return without changing anything. If true, we go through the array of sizes and set them to true.

I think it's okay not to check the provide_additional_mime_types value directly to see if it's false. It's primary purpose it to enable (not disable) this behavior anyway, and since the setting explicitly says "all", it's fine to do it for all sizes, regardless of what other configuration may be present.

felixarntz · 2024-11-21T18:21:27Z

plugins/webp-uploads/hooks.php

+	if ( webp_uploads_is_fallback_all_sizes_enabled() ) {
+		echo '<meta name="generator" content="webp-uploads-fallback-all-sizes ' . esc_attr( WEBP_UPLOADS_VERSION ) . '">' . "\n";
+	}


Why is that being added? We shouldn't have an extra generator tag for this feature specifically. If we need feature-specific indicators in the frontend, let's discuss that in a separate issue.

In the #1513 there is mention of the adding the generator tag thats why I added it.

Ideally when users enable this feature we should notate that in the Generator tag so we can later discover how many sites opt into generating all image sizes.

Should I remove the generator tag code from this PR?

Yeah, I think so, let's not do it in this PR, as it needs further discussion.

Maybe worth opening an issue to discuss whether/how we want to make more specific features discoverable in the frontend? Such an issue could link to that comment as one example reference.

Removed the generator tag code from this PR.

felixarntz · 2024-11-21T18:21:51Z

plugins/webp-uploads/settings.php

+	// Add a setting to generate fallback images in all sizes including custom sizes.
+	register_setting(
+		'media',
+		'perflab_generate_fallback_all_sizes',


Maybe rename this to perflab_generate_all_fallback_sizes, that reads a bit better.

felixarntz · 2024-11-21T18:22:06Z

plugins/webp-uploads/settings.php

+ *
+ * @since n.e.x.t
+ */
+function webp_uploads_generate_fallback_all_sizes_callback(): void {


See above in terms of naming.

felixarntz · 2024-11-21T18:23:24Z

plugins/webp-uploads/settings.php

+	// Add setting field for generating fallback images in all sizes including custom sizes.
+	add_settings_field(
+		'perflab_generate_fallback_all_sizes',
+		__( 'Output all fallback images', 'webp-uploads' ),


Suggested change

__( 'Output all fallback images', 'webp-uploads' ),

__( 'Generate all fallback image sizes', 'webp-uploads' ),

…ub.com/b1ink0/performance into add/option-to-generate-all-image-sizes

felixarntz

Thanks @b1ink0, LGTM!

Paging @adamsilverstein for additional review.

mukeshpanchal27

Overall looks good to me. Left some feedbacks.

plugins/webp-uploads/settings.php

mukeshpanchal27 · 2024-12-02T05:51:11Z

plugins/webp-uploads/helper.php

+ * @return bool True if the option is enabled, false otherwise.
+ */
+function webp_uploads_should_generate_all_fallback_sizes(): bool {
+	return (bool) get_option( 'perflab_generate_all_fallback_sizes' );


Should add default value here as well.

Suggested change

return (bool) get_option( 'perflab_generate_all_fallback_sizes' );

return (bool) get_option( 'perflab_generate_all_fallback_sizes', 0 );

mukeshpanchal27 · 2024-12-02T05:51:25Z

plugins/webp-uploads/hooks.php

+ * @param array<string, bool> $allowed_sizes A map of image size names and whether they are allowed to have additional MIME types.
+ *
+ * @return array<string, bool> Modified map of image sizes with additional MIME type support.


Suggested change

* @param array<string, bool> $allowed_sizes A map of image size names and whether they are allowed to have additional MIME types.

*

* @return array<string, bool> Modified map of image sizes with additional MIME type support.

* @param array<string, bool> $allowed_sizes A map of image size names and whether they are allowed to have additional MIME types.

* @return array<string, bool> Modified map of image sizes with additional MIME type support.

mukeshpanchal27 · 2024-12-02T05:52:05Z

plugins/webp-uploads/settings.php

+ * @since n.e.x.t
+ */
+function webp_uploads_generate_all_fallback_sizes_callback(): void {
+	$all_fallback_sizes_enabled   = 1 === (int) get_option( 'perflab_generate_all_fallback_sizes', 0 );


Suggested change

$all_fallback_sizes_enabled = 1 === (int) get_option( 'perflab_generate_all_fallback_sizes', 0 );

$all_fallback_sizes_enabled = 1 === webp_uploads_should_generate_all_fallback_sizes();

Can we re-use webp_uploads_should_generate_all_fallback_sizes() here as well?

Yes we can done in 3a64713

mukeshpanchal27 · 2024-12-02T05:58:13Z

plugins/webp-uploads/settings.php

+	$all_fallback_sizes_hidden_id = 'perflab_generate_all_fallback_sizes_hidden';
+
+	?>
+	<style>


Future request for <style> and <script> tags for individual functions:

Currently, for the <picture> element, we have added additional CSS and script tags. Can we centralize these in a common location ( Single tag for <style> and <script> )? This would ensure that in the future, we don’t need to re-add them in specific callback function.

Would moving <style> and <script> to files and enqueuing them better option for centralization then?

Something we can do in follow-up PR once these one is committed.

…lugin

adamsilverstein · 2024-12-02T15:26:54Z

plugins/webp-uploads/helper.php

+ *
+ * @since n.e.x.t
+ *
+ * @return bool True if the option is enabled, false otherwise.


Suggested change

* @return bool True if the option is enabled, false otherwise.

* @return bool Whether the option is enabled. Default is false.

adamsilverstein

Overall looks great. One small doc change suggestion. Unit tests to validate the feature works as expected would also be nice to have.

…unction

…bled

adamsilverstein

Nice work!

plugins/webp-uploads/tests/test-load.php

b1ink0 and others added 4 commits November 14, 2024 17:03

Add settings for generate fallback images in all sizes. Add functiona…

95e7878

…lity to use added new settings

Add generator tag for tracking usage of generate all fallback image s…

8473bb8

…izes option

Merge branch 'WordPress:trunk' into add/option-to-generate-all-image-…

536c799

…sizes

Remove use of empty()

fa34edf

b1ink0 marked this pull request as ready for review November 21, 2024 13:01

b1ink0 requested review from adamsilverstein, felixarntz and westonruter as code owners November 21, 2024 13:01

felixarntz requested changes Nov 21, 2024

View reviewed changes

felixarntz added this to the webp-uploads n.e.x.t milestone Nov 21, 2024

felixarntz added [Type] Enhancement A suggestion for improvement of an existing feature [Plugin] Modern Image Formats Issues for the Modern Image Formats plugin (formerly WebP Uploads) labels Nov 21, 2024

b1ink0 and others added 4 commits November 22, 2024 14:47

Change the function, option, variable names

e8460fe

Merge branch 'trunk' into add/option-to-generate-all-image-sizes

39e6004

Merge branch 'add/option-to-generate-all-image-sizes' of https://gith…

e297363

…ub.com/b1ink0/performance into add/option-to-generate-all-image-sizes

Move logic to separate function

6ddd884

b1ink0 requested a review from felixarntz November 22, 2024 12:22

b1ink0 and others added 2 commits November 25, 2024 19:31

Remove generator tag

b80a9b0

Merge branch 'trunk' into add/option-to-generate-all-image-sizes

d477128

felixarntz approved these changes Nov 26, 2024

View reviewed changes

Merge branch 'trunk' into add/option-to-generate-all-image-sizes

9eaaeec

westonruter assigned adamsilverstein Nov 29, 2024

mukeshpanchal27 reviewed Dec 2, 2024

View reviewed changes

b1ink0 added 2 commits December 2, 2024 14:55

Fix fallback option retrieval and clean up docblock in webp uploads p…

4fdbcc0

…lugin

Use existing function for getting option value

3a64713

b1ink0 requested a review from mukeshpanchal27 December 2, 2024 12:09

Merge branch 'trunk' into add/option-to-generate-all-image-sizes

546e2d7

adamsilverstein reviewed Dec 2, 2024

View reviewed changes

b1ink0 and others added 4 commits December 3, 2024 23:39

Merge branch 'trunk' into add/option-to-generate-all-image-sizes

2747d1b

Update docblock for webp_uploads_should_generate_all_fallback_sizes f…

36fbfd7

…unction

Add test case for generating fallback images for all sizes option ena…

e29a33e

…bled

Merge branch 'trunk' into add/option-to-generate-all-image-sizes

e4601ae

b1ink0 requested a review from adamsilverstein December 4, 2024 17:54

Merge branch 'trunk' into add/option-to-generate-all-image-sizes

3ed8269

b1ink0 mentioned this pull request Dec 5, 2024

Newer formats not generated for custom images sizes if Output fallback images is enabled #1720

Closed

adamsilverstein approved these changes Dec 5, 2024

View reviewed changes

mukeshpanchal27 reviewed Dec 6, 2024

View reviewed changes

plugins/webp-uploads/tests/test-load.php Show resolved Hide resolved

Cleanup test attachment after assertions

10e0c39

b1ink0 requested a review from mukeshpanchal27 December 6, 2024 13:54

mukeshpanchal27 approved these changes Dec 6, 2024

View reviewed changes

mukeshpanchal27 merged commit 20a45a3 into WordPress:trunk Dec 6, 2024
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable end user opt-in to generate all sizes in fallback format #1689

Enable end user opt-in to generate all sizes in fallback format #1689

b1ink0 commented Nov 21, 2024 •

edited

Loading

github-actions bot commented Nov 21, 2024 •

edited

Loading

felixarntz left a comment

felixarntz Nov 21, 2024

felixarntz Nov 21, 2024

felixarntz Nov 21, 2024

b1ink0 Nov 22, 2024

felixarntz Nov 23, 2024 •

edited

Loading

b1ink0 Nov 26, 2024

felixarntz Nov 21, 2024

felixarntz Nov 21, 2024

felixarntz Nov 21, 2024

felixarntz left a comment

mukeshpanchal27 left a comment

mukeshpanchal27 Dec 2, 2024

mukeshpanchal27 Dec 2, 2024

mukeshpanchal27 Dec 2, 2024

b1ink0 Dec 2, 2024

mukeshpanchal27 Dec 2, 2024

b1ink0 Dec 2, 2024

mukeshpanchal27 Dec 2, 2024

adamsilverstein Dec 2, 2024

adamsilverstein left a comment

adamsilverstein left a comment

	__( 'Output all fallback images', 'webp-uploads' ),
	__( 'Generate all fallback image sizes', 'webp-uploads' ),

	return (bool) get_option( 'perflab_generate_all_fallback_sizes' );
	return (bool) get_option( 'perflab_generate_all_fallback_sizes', 0 );

	$all_fallback_sizes_enabled = 1 === (int) get_option( 'perflab_generate_all_fallback_sizes', 0 );
	$all_fallback_sizes_enabled = 1 === webp_uploads_should_generate_all_fallback_sizes();

	* @return bool True if the option is enabled, false otherwise.
	* @return bool Whether the option is enabled. Default is false.

Enable end user opt-in to generate all sizes in fallback format #1689

Enable end user opt-in to generate all sizes in fallback format #1689

Conversation

b1ink0 commented Nov 21, 2024 • edited Loading

Summary

Relevant technical choices

Settings Page Screenshots

Sample outputs for the below custom sizes

github-actions bot commented Nov 21, 2024 • edited Loading

felixarntz 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

felixarntz Nov 23, 2024 • edited Loading

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

felixarntz left a comment

Choose a reason for hiding this comment

mukeshpanchal27 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

adamsilverstein left a comment

Choose a reason for hiding this comment

adamsilverstein left a comment

Choose a reason for hiding this comment

b1ink0 commented Nov 21, 2024 •

edited

Loading

github-actions bot commented Nov 21, 2024 •

edited

Loading

felixarntz Nov 23, 2024 •

edited

Loading