WordPress
Update `wp_calculate_image_sizes` to Reflect Changes in sizes attribute
Bug Description
As discussed in https://github.com/WordPress/performance/pull/1354#pullrequestreview-2185993737, the wp_calculate_image_sizes() function does not currently account for the changes made to the sizes attribute in PR #1329.
We need to update the sizes value in wp_calculate_image_sizes() so that any usage of this core function benefits from the improved sizes attribute.
Alternatively, we should ensure that the enhanced sizes attribute is applied via the wp_calculate_image_sizes filter, so any other code that directly calls wp_calculate_image_sizes() will also benefit from the optimizations.
Thanks for creating the issue, @mukeshpanchal27. I agree that it would be best if third party code that calls wp_calculate_image_sizes() directly would benefit from the improvements we're making. In order to do so, we would need to be able to pass a layout constraint to the function, which wouldn't be backward compatible, so applying our optimizations via the filter is probably the best approach.
@joemcgill @westonruter I have searched in WPdirectory to see if any themes or plugins use the wp_calculate_image_sizes() function to get the image sizes, but no one uses the function directly.
- Plugins: https://wpdirectory.net/search/01J445XMKVPAMTHD6VNAXFV4FD
- Themes: https://wpdirectory.net/search/01J445Y6GDKKPA3RVX5Q1Q21T1
As explored in POC #1382, we can't add wp_calculate_image_sizes filter in an accurate sizes filter without creating some issues that Weston pointed out. Can we go ahead and close this issue for now? We can re-open it if a possible solution arises.
Let me know if I missed anything.
The WP Directory search uses Regex, so you need to modify the search a little bit. Looks like there are some uses of the function after all:
- Plugins: https://wpdirectory.net/search/01J4S7MK5QT588MRPBS8HZACMV
- Themes: https://wpdirectory.net/search/01J4S7Q8VNKRY64R0BAZBMY63Y
Thanks @joemcgill. Since we don't have a next step for this, we'll move it to the next release.
@mukeshpanchal27 @joemcgill Just discovered this issue via the doc for further enhancing the sizes accuracy, I'm wondering: Why can't we enhance wp_calculate_image_sizes() to pass an array with layout constraints to it? If it's an optional parameter, I don't think it would be backward incompatible.
If there's layout constraints provided, they could be used by the function and the filter. If none are provided, it would just be an empty array, the function would behave the same it does now, and the filter would also only receive that empty array.
If we make this change in Core, I think we would need to define a bit more what the shape of those layout constraints would look like (e.g. which array keys to support etc), but I think it's worthwhile pursuing as indeed this would allow us to make the optimizations in the central function intended for this purpose, rather than in the arbitrary wp_content_img_tag filter. This could also bring the enhancements to more areas of WordPress out of the box, e.g. functions like wp_get_attachment_image() that call wp_calculate_image_sizes() directly.
This is what i and @joemcgill discussed when we exploring the possible approaches for ancestor blocks definition work.
As the beta for 6.7 is coming in few weeks, mean time can we add the new array parameter for wp_calculate_image_sizes() and also passed it to filter so we can get more time to think the keys that we can add for support?
/**
* Creates a 'sizes' attribute value for an image.
*
* @since 4.4.0
* @since 6.7.0 Added the `$layout_constraints` parameter.
*
* @param string|int[] $size Image size. Accepts any registered image size name, or an array of
* width and height values in pixels (in that order).
* @param string|null $image_src Optional. The URL to the image file. Default null.
* @param array|null $image_meta Optional. The image meta data as returned by 'wp_get_attachment_metadata()'.
* Default null.
* @param int $attachment_id Optional. Image attachment ID. Either `$image_meta` or `$attachment_id`
* is needed when using the image size name as argument for `$size`. Default 0.
* @param array $layout_constraints {
* Optional. The layout constraints for accurate calculating the sizes.
*
* @type string $block_name The block name.
* @type string $block_align The block alignment.
* @type array $column The column data.
* }
* @return string|false A valid source size value for use in a 'sizes' attribute or false.
*/
function wp_calculate_image_sizes( $size, $image_src = null, $image_meta = null, $attachment_id = 0, $layout_constraints = array() ) {
Some other key can be added in the allow list for $layout_constraints array.
WDYT?
@mukeshpanchal27 I think overall what you're proposing makes sense. Though I think we shouldn't introduce this to Core until we also have an idea about what the supported (and documented) keys would be for the $layout_constraints parameter.
If we can figure that out in time for 6.7 Beta, that's great. However, it's not super urgent because we can still work towards that solution even without the Core function adjusted: For example, if we implement our enhanced logic in the plugin, we could use a workaround, e.g. temporarily set a global or something like that which the filter then uses. This would of course be a hack, but okay for this kind of thing where we have a clean path forward for once this would be in Core.
I'm definitely open to the idea that we may need to modify the signature of wp_calculate_image_sizes() to allow for additional context to be passed.
However, it's important to consider where this function is called:
wp_get_attachment_image() – Creates standalone image markup from an attachment ID, which won't have any of the context we need.
wp_image_add_srcset_and_sizes() – Dynamically adds responsive image attributes to images included in content, which can be the block template or when filtering the_content, the_excerpt, or widget_text_content. In these cases, the call stack looks something like this:
wp_filter_content_tags()
> wp_img_tag_add_srcset_and_sizes_attr()
> wp_image_add_srcset_and_sizes()
Right now, wp_filter_content_tags() (link) takes a blob of HTML and uses preg_match to store all of the img elements to an array. Next, it iterates over the array to apply dynamic changes (e.g. adding srcset and sizes) to the img tags before using str_replace to replace the original img in the content with the modified img tag. Because of this approach, the modifications are all happening outside of the context of the full HTML structure and the context we need is not available.
To work around this, we either need to redesign the way wp_filter_content_tags() works, so that tags are being modified within the context of the HTML being filtered, or we need to move the place where wp_calculate_image_sizes() is being added to image markup to another part of the system.
From a plugin perspective, we can likely work around these limitations by adding a filter to the wp_filter_content_tags hook which passes the additional context we need before calling wp_filter_content_tags() directly. Here's an experimental gist that adds a temporary filter during block rendering to add sizes directly to an image block using attributes from the block object itself.
This works, but a more direct approach that modifies core directly would be best.
As we've been exploring moving the calculation of sizes attributes from the_content filter and directly to block rendering in #1701, it seems like the most promising approach is to add a filter to wp_calculate_image_sizes to improve the default value when we have additional layout context to use.
For this to be affective, we would need to move Core's usage of this function from a post-processing step that runs on all rendered content (as described in https://github.com/WordPress/performance/issues/1381#issuecomment-2347097053) and to part of the rendering process so the function gets called from within the layout context where the image markup is initially rendered.
Even so, I don't think we can change the existing requirements of the wp_calculate_image_sizes() function, which only requires either a size array or an intermediate size string (e.g., 'full', 'medium', etc.) along with an attachment ID to calculate a default sizes attribute, so use cases like the one that we encountered in https://github.com/WordPress/performance/issues/1349 will always produce less desirable sizes attributes than ones that can be calculated during layout rendering.
@joemcgill
For this to be affective, we would need to move Core's usage of this function from a post-processing step that runs on all rendered content
I wonder whether we really need to move it - maybe we can only add it to being called somewhere else? For instance our callback would probably include conditions anyway to only operate if the necessary layout context is present - which it would be during block rendering, but not in the post-processing step.
Even so, I don't think we can change the existing requirements of the
wp_calculate_image_sizes()function
+1, though we could add e.g. an optional $layout_constraints parameter to the function. As long as it's additional and optional, there wouldn't be any BC concerns about it.
Alternatively to the whole idea of using wp_calculate_image_sizes though, maybe the safer solution is to implement a new function specific to this purpose. FWIW even today, blocks can already add their own sizes attribute, same with other things like loading and fetchpriority, for which there are also post-processing functions.
I like to think about it as the post-processing functions being a "best guess" to provide these attributes if they aren't already there. All the existing post-processing callbacks including for the sizes attribute only inject attributes if they do not already exist, and bail early if they are already present. So IMO it would be totally reasonable to implement the new layout-specific sizes calculation in a more appropriate layer (e.g. block rendering) via a separate mechanism.
Just to be clear, I'm not opposed to using wp_calculate_image_sizes, but there may be too many limitations and BC risks, and if so, I think an entirely separate new function / filter would definitely be reasonable.