Make WordPress Core

Opened 8 years ago

Last modified 22 months ago

#34981 assigned defect (bug)

Usage of `image_size_names_choose` breaks JS attachment model attributes

Reported by: dnaber-de's profile dnaber-de Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 4.3.1
Component: Media Keywords: has-patch needs-testing
Focuses: Cc:


Here's a small use case that shows how the issue arise.

I want to limit the options of image sizes in the default media modal to so I use the filter image_size_names_choose provided in wp-admin/includes/media.php and remove the sizes thumbnail and as well as medium. This brings me the expected result:

With this I have now the problem, that for every thumbnail image in the media library (mode: grid) the large image source is used which is a performance issue if there are more than a couple of images in the library.

The reason is, that the same filter image_size_names_choose is also applied to each image in wp_prepare_attachment_for_js() and therefore the attachment model does not reflect all available image sizes (console.log( attachment.sizes ):

Here's a plugin that reproduces the issue in a clean WordPress (4.4) install:


 * Plugin Name: Image sizes issue

        function( $sizes ) {
                unset( $sizes[ 'medium' ] );
                unset( $sizes[ 'thumbnail' ] );

                return $sizes;
add_action( 'admin_enqueue_scripts', function() {

                plugins_url( '/image_sizes_issue.js', __FILE__ ),
                [ 'jquery' ]
} );


( function( $, wp ) {
        $( function()  {
                var image   = '4' );
                var promise = image.sync( 'read', image );
                promise.then( function( results ) {
                        console.log( results.sizes );
                } );
         } );
} )(
        window.wp = window.wp || {}

For what I see, the filter image_size_names_choose is meant to affect the default UI for customization purpose. At least the name of the filter and the name of the function image_size_input_fields() (introduced in 2.7) suggests that.

In contrast to that, wp_prepare_attachment_for_js() prepares the data structure for the Javascript API. I'm sure there's some reason to apply the filter there, but I have no idea what this reason could be.

A possible, backward compatible solution could be, to pass a second argument to the filter to specify the context, the filter is applied in.

I detected this behaviour in 4.3.1 but I think it go back to 3.5 when wp_prepare_attachments_for_js() was introduced.

Attachments (5)

34981.diff (3.4 KB) - added by wonderboymusic 8 years ago.
34981.2.diff (691 bytes) - added by jorbin 8 years ago.
34981.3.diff (959 bytes) - added by azaozz 8 years ago.
34981.4.diff (1.6 KB) - added by joemcgill 8 years ago.
34981.5.diff (2.2 KB) - added by desrosj 22 months ago.

Download all attachments as: .zip

Change History (25)

#1 @ocean90
8 years ago

The filter was added in [22850], #30150 suggests to add a context to each filter call.

8 years ago

#2 @wonderboymusic
8 years ago

  • Keywords has-patch needs-docs added
  • Milestone changed from Awaiting Review to 4.7

34981.diff adds context

#3 @dnaber-de
8 years ago

Using function and method names for a context isn't a good idea IMO. It ties the filter (and thus the filtering code) to where it is actually called. That makes refactorings of the code almost impossible in a sane way. If you want to refactor Custom_Background::wp_set_background_image() later, you'll have to keep the second parameter to the value 'Custom_Background::wp_set_background_image'.
The method itself is marked as deprecated so a refactoring is likely:

 * @since 3.4.0
 * @deprecated 3.5.0

I think we need to declare concrete contexts and name them independently of where the contexts apply. A comparable situation is the context parameter or sanitize_post().

I see actually only two contexts here:

  • the select form elements (that one, the filter was originally meant for) in WordPress core's media modal views (another differentiation of these could also be done via JavaScript).
  • the data sanitizing for JS APIs in wp_prepare_attachment_for_js().

(However, all this is just a hack-ish workaround for backward compatibility. I see no reason why wp_prepare_attachment_for_js() should apply this filter anyway.)

8 years ago

#4 @jorbin
8 years ago

  • Keywords needs-patch added; has-patch removed

Thought for a second that maybe the image_size_names_choose filter could just be removed, but that would reintroduce #22598.

I think context is going to be the key, but I agree with @dnaber-de that it shouldn't be tied to the name of the function, but to the actual context.

#5 @joemcgill
8 years ago

I don't think that removing the image_size_names_choose filter would reintroduce #22598 as the main issue there is that people might be using the image_downsize filter to short circuit WP's native image resizing implementation.

Besides wp_prepare_attachment_for_js(), the image_size_names_choose filter is applied in three other places:

  • image_size_input_fields() – only called from legacy media upload forms.
  • Custom_Background::wp_set_background_image() – deprecated in 3.5
  • wp_print_media_templates() – three times, and always used to populate size options for a select element.

If wp_prepare_attachment_for_js() is going to continue to be used as a standard way to create generalized attachment models to return in AJAX requests, then we should probably consider moving this filter to the front-end where interfaces are being populated by size options, rather than doing so when the model is built.

If that approach turns out to be impossible due to backwards compatibility issues, it might be better pass a second parameter to wp_prepare_attachment_for_js() which would give requests an opportunity to bypass this filter and return all available image sizes.

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

8 years ago

#7 @stevenkword
8 years ago

  • Keywords has-patch needs-testing added; needs-docs needs-patch removed

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

8 years ago

8 years ago

#9 @azaozz
8 years ago

I also think the use of image_size_names_choose in wp_prepare_attachment_for_js() is wrong. In this case it controls what image sizes are outputted to JS, i.e. has little to do with the original purpose of only filtering what is displayed in the UI.

However removing it may introduce other errors. For example if a plugin adds more sizes, they will not be available in JS.

We already always add (hard-code) the full size. A simple workaround would be to ensure that the thumbnail size is always present so it can be used in the media modal and library to display the image thumbnails. See 34981.3.diff.

A better way to fix this would be to add srcset to these thumbnails :)

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

8 years ago

#11 @helen
8 years ago

Seems like this needs some cursory research on what people are doing in the wild with image_size_names_choose and what other filters people use or could use to add/remove image sizes. If people are using this filter and actually counting on it removing those sizes from the attachment model then we should know that first. In a vacuum, it seems like it shouldn't apply to wp_prepare_attachment_for_js() at all.

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

8 years ago

This ticket was mentioned in Slack in #core-images by mike. View the logs.

8 years ago

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

8 years ago

#15 @jbpaul17
8 years ago

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

8 years ago

#16 @joemcgill
8 years ago

There are ~60 plugins in the .org repo that are filtering on image_size_names_choose — overwhelmingly to add additional image sizes to the insert image dialogue, which is already applying the image_size_names_choose filter when the attachment-display-settings template is created in wp_print_media_templates(). I think we should be fine removing the filter from wp_prepare_attachment_for_js().

In 34981.4.diff, I've skipped the image_size_names_choose filter whenever we're returning sizes from the attachment metadata and am now only using it when we're looking to see if someone is setting sizes by filtering image_downsize. This allows all available image sizes to be returned by wp_prepare_attachment_for_js(). All Media, AJAX, and Customize tests are still passing.

Regarding the unintended consequences in the media library whenever someone removes appropriate sizes, I agree that we should look into using srcset here, but we should also look at converting the media library to use the new JSON REST API endpoints instead of using admin-ajax.

#17 @joemcgill
8 years ago

  • Keywords 4.8-early added
  • Milestone changed from 4.7 to Future Release

Let's pick this up early in the 4.8 cycle.

#18 @johnbillion
2 years ago

  • Keywords 4.8-early removed

22 months ago

#19 @desrosj
22 months ago

  • Milestone set to Future Release

Came across this one because it was missing a milestone.

I've refreshed the most recent patch in 34981.5.diff and done some basic testing, and nothing seems to have broken.

I haven't taken the deep dive into the code to make sure this still makes sense, but I did perform a new search of the plugin directory. It looks like there are now 471 occurrences of image_size_names_choose in the directory in 297 different plugins. However, a lot of these are false positives (plugins calling apply_filters( 'image_size_names_choose' ) manually, and a good handful are bundling an older version of Advanced Custom Fields which references the filter and actually calls out oddities mentioned here.

I also checked the Gutenberg code base, and while the filter is not used in the source code, it looks like it's used within end to end tests.

I'm resetting the Owned by field because it has been 6+ years without any progress. Sometimes contributors shy away from tickets with owners.

If @joemcgill or any past participants feel like running with this one, please feel free to self assign again.

#20 @joemcgill
22 months ago

  • Owner joemcgill deleted

Thanks @desrosj! Happy to have someone else take this over as clearly it's not something I'm actively working on.

Note: See TracTickets for help on using tickets.