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 decoding as a default argument #5331

Closed

Conversation

pereirinha
Copy link
Member

This PR fixes a regression introduced on #58892 when moving the responsibility for the decoding attribute to the wp_get_loading_optimization_attributes.

This will add the decoding attribute to the get_avatar(). As trunk currently is, this context will fail to get any optimization attributes while running the_content filter.

Trac ticket: #58892


This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

@@ -2845,6 +2845,7 @@ function get_avatar( $id_or_email, $size = 96, $default_value = '', $alt = '', $
'loading' => null,
'fetchpriority' => null,
'extra_attr' => '',
'decoding' => 'async',
Copy link
Member

Choose a reason for hiding this comment

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

The default should be null (unset), as is for the other loading optimization attributes. Otherwise this partially reverts the improvement from 58892.

Copy link
Member

Choose a reason for hiding this comment

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

@felixarntz, it appears that the get_avatar function does not invoke any separate function to generate the loading optimization attributes. Instead, it constructs this code within the same function. Please review the code at this location: https://github.com/WordPress/wordpress-develop/blob/6.3/src/wp-includes/pluggable.php#L2938-L2946.

If we provide null as an argument, it should now work for the get_avatar function within the Block Avatar block.

Please let me know if I have misunderstood this.

Copy link
Member

Choose a reason for hiding this comment

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

@mukeshpanchal27

it appears that the get_avatar function does not invoke any separate function to generate the loading optimization attributes.

It does, see line 2869. That's why we should not set async here, it needs to come from that function call.

Copy link
Member

Choose a reason for hiding this comment

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

Hello @pereirinha and @felixarntz, According to the information provided in the WordPress core file at https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-includes/pluggable.php#L2868, it appears that the context for get_avatar is not the_content. As a result, it return an empty array for both the 'Avatar' and 'Author' editor blocks.

Could you please confirm whether this is working as expected on your end?

Copy link
Member

Choose a reason for hiding this comment

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

@mukeshpanchal27 That is okay: Images in blocks that are part of the_content are intentionally skipped, as they should only be considered later as part of the whole content blob (with the_content context).

@pereirinha
Copy link
Member Author

@felixarntz @mukeshpanchal27

I made the change and didn't find any issues.

Copy link
Member

@mukeshpanchal27 mukeshpanchal27 left a comment

Choose a reason for hiding this comment

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

Check my comment #5331 (comment)

@felixarntz
Copy link
Member

@felixarntz felixarntz closed this Oct 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants