WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#34379 closed defect (bug) (fixed)

Responsive Images: Streamline wp_get_attachment_image_sizes()

Reported by: joemcgill Owned by: joemcgill
Milestone: 4.4 Priority: normal
Severity: normal Version: 4.4
Component: Media Keywords: has-patch commit
Focuses: performance Cc:

Description

While working on performance optimizations for wp_get_attachment_image_sizes() I came to the conclusion that we could drastically simplify the internals of this function without losing any useful functionality. Originally, the third parameter of this function would allow a developer to pass in an array of source sizes information which would be used to build the sizes attribute. However, the logic wasn't transparent and in all cases where this functionality would be useful, it would be far easier for a developer to simply build the sizes value themselves.

Instead, I'm proposing that the third parameter be used to pass in the width of the image, which has performance benefits by not making us recalculate a width that is already known. I'd also like to move the placement of the filter to the returned value and change the filter name to 'wp_get_attachment_image_sizes' to be consistent with other filter names in this family of functions.

I've updated the applicable tests and inline docs.

Aside: @jaspermdegroot was integral in the crafting of this patch and should be included in any props.

Attachments (3)

34379.patch (9.8 KB) - added by joemcgill 6 years ago.
34379.2.patch (12.6 KB) - added by joemcgill 6 years ago.
34379.3.patch (3.4 KB) - added by SergeyBiryukov 6 years ago.

Download all attachments as: .zip

Change History (18)

#1 @joemcgill
6 years ago

  • Owner set to joemcgill
  • Status changed from new to assigned

#2 @mikeschroder
6 years ago

  • Focuses performance added
  • Type changed from enhancement to defect (bug)

#3 @mikeschroder
6 years ago

  • Milestone changed from Awaiting Review to 4.4

This ticket was mentioned in Slack in #feature-respimg by joemcgill. View the logs.


6 years ago

@joemcgill
6 years ago

#5 @joemcgill
6 years ago

  • Keywords commit added

@joemcgill
6 years ago

#6 @joemcgill
6 years ago

34379.2.patch Cleans up some inline docs and makes sure a srcset attribute doesn't get set without a corresponding sizes attribute in wp_get_attachment_image().

This ticket was mentioned in Slack in #core by joemcgill. View the logs.


6 years ago

#8 @wonderboymusic
6 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 35355:

Media: in wp_get_attachment_image_sizes(), to streamline and for performance:

  • Change the 3rd arg from args to width
  • Change wp_image_sizes_args filter to wp_get_attachment_image_sizes

Updates unit tests.

Props joemcgill.
Fixes #34379.

#9 @jorbin
6 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

I really don't like the idea of changing the wp_get_attachment_image_sizes function signature. If we need to go from accepting an array to accepting an integer, we should either:

1) Create a new function and deprecated wp_get_attachment_image_sizes
2) Ensure that passing an array doesn't change the behavior of wp_get_attachment_image_sizes through automated tests.

Backwards incompatible changes like this make it harder for users to upgrade and break sites and plugins when they do upgrade.

#10 @joemcgill
6 years ago

@jorbin to be clear, wp_get_attachment_image_sizes() is a new function introduced in 34855 to create a sizes attribute for responsive images. As such, I'm not sure I see the harm in changing the signature of this function.

#11 @wonderboymusic
6 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

We aren't even at Beta - the function signature can change

#12 @jorbin
6 years ago

I missed the @since when reviewing the changeset. Sorry about that.

#13 @SergeyBiryukov
6 years ago

In 35402:

Remove assignments from conditions in wp_get_attachment_image() and wp_img_add_srcset_and_sizes().

See #34379.

#14 @SergeyBiryukov
6 years ago

In 35404:

After [35402], don't unnecessary run wp_get_attachment_image_srcset() and wp_get_attachment_image_sizes() in wp_get_attachment_image() if srcset is passed as an argument.

See #34379.

#15 @SergeyBiryukov
6 years ago

In 35405:

After [35402], don't unnecessary run wp_get_attachment_metadata(), wp_get_attachment_image_srcset(), and wp_get_attachment_image_sizes() in wp_img_add_srcset_and_sizes().

See #34379.

Note: See TracTickets for help on using tickets.