WordPress.org

Make WordPress Core

Opened 2 years ago

Last modified 13 months ago

#34981 assigned defect (bug)

Usage of `image_size_names_choose` breaks JS attachment model attributes

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

Description

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:

https://naber.pegasus.uberspace.de/fs/public/images/2015/limit-attachment-sizes.png

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

https://naber.pegasus.uberspace.de/fs/public/images/2015/attachment-model-sizes.png

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.

Attachments (4)

34981.diff (3.4 KB) - added by wonderboymusic 15 months ago.
34981.2.diff (691 bytes) - added by jorbin 15 months ago.
34981.3.diff (959 bytes) - added by azaozz 14 months ago.
34981.4.diff (1.6 KB) - added by joemcgill 13 months ago.

Download all attachments as: .zip

Change History (21)

#1 @ocean90
2 years ago

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

#2 @wonderboymusic
15 months ago

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

34981.diff adds context

#3 @dnaber-de
15 months 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.)

@jorbin
15 months ago

#4 @jorbin
15 months 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
15 months 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.


14 months ago

#7 @stevenkword
14 months 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.


14 months ago

@azaozz
14 months ago

#9 @azaozz
14 months 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.


14 months ago

#11 @helen
14 months 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.


13 months ago

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


13 months ago

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


13 months ago

#15 @jbpaul17
13 months ago

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

@joemcgill
13 months ago

#16 @joemcgill
13 months 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
13 months 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.

Note: See TracTickets for help on using tickets.