Usage of `image_size_names_choose` breaks JS attachment model attributes

[dnaber-de]

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:

wp-content/plugins/image_sizes_issue/image_sizes_issue.php:

<?php
/**
 * Plugin Name: Image sizes issue
 */

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

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

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

wp-content/plugins/image_sizes_issue/image_sizes_issue.js:

( function( $, wp ) {
        $( function()  {
                var image   = wp.media.model.Attachment.get( '4' );
                var promise = image.sync( 'read', image );
                promise.then( function( results ) {
                        console.log( results.sizes );
                } );
         } );
} )(
        jQuery,
        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.

[ocean90]

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

[wonderboymusic]

34981.diff​ adds context

[dnaber-de]

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.)

[jorbin]

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.

[joemcgill]

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.

[azaozz]

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 :)

[helen]

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.

[joemcgill]

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.

[joemcgill]

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

[desrosj]

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.

[joemcgill]

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