Make WordPress Core

Opened 8 years ago

Closed 8 years ago

#40010 closed defect (bug) (fixed)

Customize: Template for site icon control fails to check if full image size exists before using

Reported by: aussieguy123's profile aussieguy123 Owned by: westonruter's profile westonruter
Milestone: 4.7.4 Priority: normal
Severity: normal Version: 4.5
Component: Customize Keywords: has-patch fixed-major
Focuses: Cc:

Description

Looks like #36749 has shown itself again in the latest core commits.

I've attached a patch, which fixes this issue.

Seems that the template is not checking if data.attachment.sizes.full exists before printing out data.attachment.sizes.full.url. This causes a console error (trying to get property url from a non-object).

To reproduce:

  • Open Customizer
  • Under site identity upload an image
  • Publish
  • Refresh the page

Expected:
Page loads normally with new icon

Actual:
Page partially loads, customizer is broken

Attachments (2)

fix_customizer.diff (1.4 KB) - added by aussieguy123 8 years ago.
40010.1.diff (1.4 KB) - added by westonruter 8 years ago.

Download all attachments as: .zip

Change History (23)

#1 @aussieguy123
8 years ago

Heres the exact js error:

Uncaught TypeError: Cannot read property 'url' of undefined

at eval (eval at m.template (underscore.min.js?ver=1.8.3:5), <anonymous>:23:35)
at c (underscore.min.js?ver=1.8.3:5)
at wp-util.js?ver=4.8-alpha-40139:34
at child.renderContent (customize-controls.js?ver=4.8-alpha-40139:2223)
at Object.<anonymous> (customize-controls.js?ver=4.8-alpha-40139:1974)
at i (jquery.js?ver=1.12.4:2)
at Object.add [as done] (jquery.js?ver=1.12.4:2)
at Function.<anonymous> (customize-controls.js?ver=4.8-alpha-40139:1970)
at i (jquery.js?ver=1.12.4:2)
at Object.fireWith [as resolveWith] (jquery.js?ver=1.12.4:2)

#2 @westonruter
8 years ago

@aussieguy123 This is only happening in a context where Jetpack's Photon service is being used, right?

Does your patch fix the problem or just the symptom? In other words, if image sizes are being dynamically generated, is the original image going to be shown?

It could be that there is actually a Jetpack issue where where it should be filtering wp_prepare_attachment_for_js to supply all of the added image sizes with URLs off to the WordPress.com endpoints.

#3 @westonruter
8 years ago

@aussieguy123 also, if you open up your console, what does _wpCustomizeSettings.controls.site_icon.attachment output? I'm expecting it to have an empty sizes property. But I want to know what the other properties are as well. Please attach the JSON.

#4 @westonruter
8 years ago

  • Component changed from General to Customize
  • Keywords reporter-feedback added

#5 @aussieguy123
8 years ago

@westonruter

here is the output of _wpCustomizeSettings.controls.site_icon.attachment . Sizes is actually not empty, but the "full" size is missing.

The fix addresses the symptom. If the root cause is no full size being generated, that should be fixed seperately.

You are right that jetpack is involved. Deactivating jetpack (wp plugin deactivate jetpack --network) works around the problem. The other way around it is to delete the site_icon option ( wp --url=<yoursite.local> option delete site_icon ).

{
  "id": 44,
  "title": "cropped-mars-1.jpg",
  "filename": "cropped-mars-1.jpg",
  "url": "http://heraldsun.local/wp-content/uploads/sites/10/2017/03/cropped-mars-1.jpg",
  "link": "http://heraldsun.local/cropped-mars-1-jpg",
  "alt": "",
  "author": "1",
  "description": "http://heraldsun.local/wp-content/uploads/sites/10/2017/03/cropped-mars-1.jpg",
  "caption": "",
  "name": "cropped-mars-1-jpg",
  "status": "inherit",
  "uploadedTo": 0,
  "date": 1488348427000,
  "modified": 1488348427000,
  "menuOrder": 0,
  "mime": "image/jpeg",
  "type": "image",
  "subtype": "jpeg",
  "icon": "http://heraldsun.local/wp-includes/images/media/default.png",
  "dateFormatted": "March 1, 2017",
  "nonces": {
    "update": "4f78209afe",
    "delete": "cc6e068550",
    "edit": "41f6cf352f"
  },
  "editLink": "http://heraldsun.local/wp-admin/post.php?post=44&action=edit",
  "meta": false,
  "authorName": "wordpress",
  "filesizeInBytes": 38387,
  "filesizeHumanReadable": "37 KB",
  "height": 512,
  "width": 512,
  "orientation": "landscape",
  "sizes": {
    "spp_thumbnail": {
      "height": 75,
      "width": 100,
      "url": "http://heraldsun.local/wp-content/uploads/sites/10/2017/03/cropped-mars-1-100x75.jpg",
      "orientation": "landscape"
    },
    "promo_thumbnail": {
      "height": 86,
      "width": 149,
      "url": "http://heraldsun.local/wp-content/uploads/sites/10/2017/03/cropped-mars-1-149x86.jpg",
      "orientation": "landscape"
    },
    "story_crop": {
      "height": 237,
      "width": 316,
      "url": "http://heraldsun.local/wp-content/uploads/sites/10/2017/03/cropped-mars-1-316x237.jpg",
      "orientation": "landscape"
    },
    "story_portrait": {
      "height": 421,
      "width": 316,
      "url": "http://heraldsun.local/wp-content/uploads/sites/10/2017/03/cropped-mars-1-316x421.jpg",
      "orientation": "portrait"
    },
    "gallery_portrait": {
      "height": 488,
      "width": 366,
      "url": "http://heraldsun.local/wp-content/uploads/sites/10/2017/03/cropped-mars-1-366x488.jpg",
      "orientation": "portrait"
    },
    "gallery_alt_image": {
      "height": 433,
      "width": 288,
      "url": "http://heraldsun.local/wp-content/uploads/sites/10/2017/03/cropped-mars-1-288x433.jpg",
      "orientation": "portrait"
    },
    "tablet_thumbnail": {
      "height": 110,
      "width": 196,
      "url": "http://heraldsun.local/wp-content/uploads/sites/10/2017/03/cropped-mars-1-196x110.jpg",
      "orientation": "landscape"
    },
    "tablet_square_small": {
      "height": 234,
      "width": 234,
      "url": "http://heraldsun.local/wp-content/uploads/sites/10/2017/03/cropped-mars-1-234x234.jpg",
      "orientation": "landscape"
    }
  },
  "compat": {
    "item": "<input type=\"hidden\" name=\"attachments[44][menu_order]\" value=\"0\" /><p class=\"media-types media-types-required-info\">Required fields are marked <span class=\"required\">*</span></p>\n\t\t\t<table class=\"compat-attachment-fields\">\t\t<tr class='compat-field-wpcom_thumbnails'>\t\t\t<th scope='row' class='label'><label for='attachments-44-wpcom_thumbnails'><span class='alignleft'>Thumbnail Images</span><br class='clear' /></label></th>\n\t\t\t<td class='field'><p class=\"hide-if-js\">You need to enable Javascript to use this functionality.</p><input type=\"button\" class=\"hide-if-no-js button\" onclick=\"jQuery(this).hide();jQuery('#wpcom-thumbs-44').slideDown('slow');\" value=\"Show Thumbnails\" /><div id=\"wpcom-thumbs-44\" class=\"hidden\"><p>Click on a thumbnail image to modify it. Each thumbnail has likely been scaled down in order to fit nicely into a grid.<br /><strong>Only thumbnails that are cropped are shown.</strong> Other sizes are hidden because they will be scaled to fit.</p><div><div style=\"float:left;margin:0 20px 20px 0;min-width:250px;\"><a href=\"http://heraldsun.local/wp-admin/admin.php?action=wpcom_thumbnail_edit&#038;id=44&#038;size=thumbnail\" target=\"_blank\"><strong>thumbnail</strong><br /><img src=\"http://heraldsun.local/wp-content/uploads/sites/10/2017/03/cropped-mars-1-150x150.jpg\" alt=\"thumbnail\" /></a></div><div style=\"float:left;margin:0 20px 20px 0;min-width:250px;\"><a href=\"http://heraldsun.local/wp-admin/admin.php?action=wpcom_thumbnail_edit&#038;id=44&#038;size=spp_thumbnail\" target=\"_blank\"><strong>spp_thumbnail</strong><br /><img src=\"http://heraldsun.local/wp-content/uploads/sites/10/2017/03/cropped-mars-1-100x75.jpg\" alt=\"spp_thumbnail\" /></a></div><div style=\"float:left;margin:0 20px 20px 0;min-width:250px;\"><a href=\"http://heraldsun.local/wp-admin/admin.php?action=wpcom_thumbnail_edit&#038;id=44&#038;size=promo_thumbnail\" target=\"_blank\"><strong>promo_thumbnail</strong><br /><img src=\"http://heraldsun.local/wp-content/uploads/sites/10/2017/03/cropped-mars-1-149x86.jpg\" alt=\"promo_thumbnail\" /></a></div><div style=\"float:left;margin:0 20px 20px 0;min-width:250px;\"><a href=\"http://heraldsun.local/wp-admin/admin.php?action=wpcom_thumbnail_edit&#038;id=44&#038;size=story_crop\" target=\"_blank\"><strong>story_crop</strong><br /><img src=\"http://heraldsun.local/wp-content/uploads/sites/10/2017/03/cropped-mars-1-316x237.jpg\" alt=\"story_crop\" /></a></div><div style=\"float:left;margin:0 20px 20px 0;min-width:250px;\"><a href=\"http://heraldsun.local/wp-admin/admin.php?action=wpcom_thumbnail_edit&#038;id=44&#038;size=story_portrait\" target=\"_blank\"><strong>story_portrait</strong><br /><img src=\"http://heraldsun.local/wp-content/uploads/sites/10/2017/03/cropped-mars-1-316x421.jpg\" alt=\"story_portrait\" /></a></div><div style=\"float:left;margin:0 20px 20px 0;min-width:250px;\"><a href=\"http://heraldsun.local/wp-admin/admin.php?action=wpcom_thumbnail_edit&#038;id=44&#038;size=major_event\" target=\"_blank\"><strong>major_event</strong><br /><img src=\"http://heraldsun.local/wp-content/uploads/sites/10/2017/03/cropped-mars-1-512x366.jpg\" alt=\"major_event\" /></a></div><div style=\"float:left;margin:0 20px 20px 0;min-width:250px;\"><a href=\"http://heraldsun.local/wp-admin/admin.php?action=wpcom_thumbnail_edit&#038;id=44&#038;size=gallery_portrait\" target=\"_blank\"><strong>gallery_portrait</strong><br /><img src=\"http://heraldsun.local/wp-content/uploads/sites/10/2017/03/cropped-mars-1-366x488.jpg\" alt=\"gallery_portrait\" /></a></div><div style=\"float:left;margin:0 20px 20px 0;min-width:250px;\"><a href=\"http://heraldsun.local/wp-admin/admin.php?action=wpcom_thumbnail_edit&#038;id=44&#038;size=gallery_landscape\" target=\"_blank\"><strong>gallery_landscape</strong><br /><img src=\"http://heraldsun.local/wp-content/uploads/sites/10/2017/03/cropped-mars-1-512x488.jpg\" alt=\"gallery_landscape\" /></a></div><div style=\"float:left;margin:0 20px 20px 0;min-width:250px;\"><a href=\"http://heraldsun.local/wp-admin/admin.php?action=wpcom_thumbnail_edit&#038;id=44&#038;size=gallery_landscape_alt\" target=\"_blank\"><strong>gallery_landscape_alt</strong><br /><img src=\"http://heraldsun.local/wp-content/uploads/sites/10/2017/03/cropped-mars-1-512x433.jpg\" alt=\"gallery_landscape_alt\" /></a></div><div style=\"float:left;margin:0 20px 20px 0;min-width:250px;\"><a href=\"http://heraldsun.local/wp-admin/admin.php?action=wpcom_thumbnail_edit&#038;id=44&#038;size=gallery_square\" target=\"_blank\"><strong>gallery_square</strong><br /><img src=\"http://heraldsun.local/wp-content/uploads/sites/10/2017/03/cropped-mars-1.jpg\" alt=\"gallery_square\" /></a></div><div style=\"float:left;margin:0 20px 20px 0;min-width:250px;\"><a href=\"http://heraldsun.local/wp-admin/admin.php?action=wpcom_thumbnail_edit&#038;id=44&#038;size=venti\" target=\"_blank\"><strong>venti</strong><br /><img src=\"http://heraldsun.local/wp-content/uploads/sites/10/2017/03/cropped-mars-1.jpg\" alt=\"venti\" /></a></div><div style=\"float:left;margin:0 20px 20px 0;min-width:250px;\"><a href=\"http://heraldsun.local/wp-admin/admin.php?action=wpcom_thumbnail_edit&#038;id=44&#038;size=tablet_landscape\" target=\"_blank\"><strong>tablet_landscape</strong><br /><img src=\"http://heraldsun.local/wp-content/uploads/sites/10/2017/03/cropped-mars-1.jpg\" alt=\"tablet_landscape\" /></a></div><div style=\"float:left;margin:0 20px 20px 0;min-width:250px;\"><a href=\"http://heraldsun.local/wp-admin/admin.php?action=wpcom_thumbnail_edit&#038;id=44&#038;size=gallery_alt_image\" target=\"_blank\"><strong>gallery_alt_image</strong><br /><img src=\"http://heraldsun.local/wp-content/uploads/sites/10/2017/03/cropped-mars-1-288x433.jpg\" alt=\"gallery_alt_image\" /></a></div><div style=\"float:left;margin:0 20px 20px 0;min-width:250px;\"><a href=\"http://heraldsun.local/wp-admin/admin.php?action=wpcom_thumbnail_edit&#038;id=44&#038;size=tablet_portrait\" target=\"_blank\"><strong>tablet_portrait</strong><br /><img src=\"http://heraldsun.local/wp-content/uploads/sites/10/2017/03/cropped-mars-1.jpg\" alt=\"tablet_portrait\" /></a></div><div style=\"float:left;margin:0 20px 20px 0;min-width:250px;\"><a href=\"http://heraldsun.local/wp-admin/admin.php?action=wpcom_thumbnail_edit&#038;id=44&#038;size=tablet_square\" target=\"_blank\"><strong>tablet_square</strong><br /><img src=\"http://heraldsun.local/wp-content/uploads/sites/10/2017/03/cropped-mars-1.jpg\" alt=\"tablet_square\" /></a></div><div style=\"float:left;margin:0 20px 20px 0;min-width:250px;\"><a href=\"http://heraldsun.local/wp-admin/admin.php?action=wpcom_thumbnail_edit&#038;id=44&#038;size=tablet_wide\" target=\"_blank\"><strong>tablet_wide</strong><br /><img src=\"http://heraldsun.local/wp-content/uploads/sites/10/2017/03/cropped-mars-1.jpg\" alt=\"tablet_wide\" /></a></div><div style=\"float:left;margin:0 20px 20px 0;min-width:250px;\"><a href=\"http://heraldsun.local/wp-admin/admin.php?action=wpcom_thumbnail_edit&#038;id=44&#038;size=tablet_thumbnail\" target=\"_blank\"><strong>tablet_thumbnail</strong><br /><img src=\"http://heraldsun.local/wp-content/uploads/sites/10/2017/03/cropped-mars-1-196x110.jpg\" alt=\"tablet_thumbnail\" /></a></div><div style=\"float:left;margin:0 20px 20px 0;min-width:250px;\"><a href=\"http://heraldsun.local/wp-admin/admin.php?action=wpcom_thumbnail_edit&#038;id=44&#038;size=tablet_gallery_landscape\" target=\"_blank\"><strong>tablet_gallery_landscape</strong><br /><img src=\"http://heraldsun.local/wp-content/uploads/sites/10/2017/03/cropped-mars-1-512x488.jpg\" alt=\"tablet_gallery_landscape\" /></a></div><div style=\"float:left;margin:0 20px 20px 0;min-width:250px;\"><a href=\"http://heraldsun.local/wp-admin/admin.php?action=wpcom_thumbnail_edit&#038;id=44&#038;size=tablet_square_small\" target=\"_blank\"><strong>tablet_square_small</strong><br /><img src=\"http://heraldsun.local/wp-content/uploads/sites/10/2017/03/cropped-mars-1-234x234.jpg\" alt=\"tablet_square_small\" /></a></div><div style=\"float:left;margin:0 20px 20px 0;min-width:250px;\"><a href=\"http://heraldsun.local/wp-admin/admin.php?action=wpcom_thumbnail_edit&#038;id=44&#038;size=spp_thumbnail\" target=\"_blank\"><strong>spp_thumbnail</strong><br /><img src=\"http://heraldsun.local/wp-content/uploads/sites/10/2017/03/cropped-mars-1-100x75.jpg\" alt=\"spp_thumbnail\" /></a></div><div style=\"float:left;margin:0 20px 20px 0;min-width:250px;\"><a href=\"http://heraldsun.local/wp-admin/admin.php?action=wpcom_thumbnail_edit&#038;id=44&#038;size=promo_thumbnail\" target=\"_blank\"><strong>promo_thumbnail</strong><br /><img src=\"http://heraldsun.local/wp-content/uploads/sites/10/2017/03/cropped-mars-1-149x86.jpg\" alt=\"promo_thumbnail\" /></a></div><div style=\"float:left;margin:0 20px 20px 0;min-width:250px;\"><a href=\"http://heraldsun.local/wp-admin/admin.php?action=wpcom_thumbnail_edit&#038;id=44&#038;size=story_crop\" target=\"_blank\"><strong>story_crop</strong><br /><img src=\"http://heraldsun.local/wp-content/uploads/sites/10/2017/03/cropped-mars-1-316x237.jpg\" alt=\"story_crop\" /></a></div><div style=\"float:left;margin:0 20px 20px 0;min-width:250px;\"><a href=\"http://heraldsun.local/wp-admin/admin.php?action=wpcom_thumbnail_edit&#038;id=44&#038;size=story_portrait\" target=\"_blank\"><strong>story_portrait</strong><br /><img src=\"http://heraldsun.local/wp-content/uploads/sites/10/2017/03/cropped-mars-1-316x421.jpg\" alt=\"story_portrait\" /></a></div><div style=\"float:left;margin:0 20px 20px 0;min-width:250px;\"><a href=\"http://heraldsun.local/wp-admin/admin.php?action=wpcom_thumbnail_edit&#038;id=44&#038;size=major_event\" target=\"_blank\"><strong>major_event</strong><br /><img src=\"http://heraldsun.local/wp-content/uploads/sites/10/2017/03/cropped-mars-1-512x366.jpg\" alt=\"major_event\" /></a></div><div style=\"float:left;margin:0 20px 20px 0;min-width:250px;\"><a href=\"http://heraldsun.local/wp-admin/admin.php?action=wpcom_thumbnail_edit&#038;id=44&#038;size=gallery_portrait\" target=\"_blank\"><strong>gallery_portrait</strong><br /><img src=\"http://heraldsun.local/wp-content/uploads/sites/10/2017/03/cropped-mars-1-366x488.jpg\" alt=\"gallery_portrait\" /></a></div><div style=\"float:left;margin:0 20px 20px 0;min-width:250px;\"><a href=\"http://heraldsun.local/wp-admin/admin.php?action=wpcom_thumbnail_edit&#038;id=44&#038;size=gallery_landscape\" target=\"_blank\"><strong>gallery_landscape</strong><br /><img src=\"http://heraldsun.local/wp-content/uploads/sites/10/2017/03/cropped-mars-1-512x488.jpg\" alt=\"gallery_landscape\" /></a></div><div style=\"float:left;margin:0 20px 20px 0;min-width:250px;\"><a href=\"http://heraldsun.local/wp-admin/admin.php?action=wpcom_thumbnail_edit&#038;id=44&#038;size=gallery_landscape_alt\" target=\"_blank\"><strong>gallery_landscape_alt</strong><br /><img src=\"http://heraldsun.local/wp-content/uploads/sites/10/2017/03/cropped-mars-1-512x433.jpg\" alt=\"gallery_landscape_alt\" /></a></div><div style=\"float:left;margin:0 20px 20px 0;min-width:250px;\"><a href=\"http://heraldsun.local/wp-admin/admin.php?action=wpcom_thumbnail_edit&#038;id=44&#038;size=gallery_square\" target=\"_blank\"><strong>gallery_square</strong><br /><img src=\"http://heraldsun.local/wp-content/uploads/sites/10/2017/03/cropped-mars-1.jpg\" alt=\"gallery_square\" /></a></div><div style=\"float:left;margin:0 20px 20px 0;min-width:250px;\"><a href=\"http://heraldsun.local/wp-admin/admin.php?action=wpcom_thumbnail_edit&#038;id=44&#038;size=venti\" target=\"_blank\"><strong>venti</strong><br /><img src=\"http://heraldsun.local/wp-content/uploads/sites/10/2017/03/cropped-mars-1.jpg\" alt=\"venti\" /></a></div><div style=\"float:left;margin:0 20px 20px 0;min-width:250px;\"><a href=\"http://heraldsun.local/wp-admin/admin.php?action=wpcom_thumbnail_edit&#038;id=44&#038;size=tablet_landscape\" target=\"_blank\"><strong>tablet_landscape</strong><br /><img src=\"http://heraldsun.local/wp-content/uploads/sites/10/2017/03/cropped-mars-1.jpg\" alt=\"tablet_landscape\" /></a></div><div style=\"float:left;margin:0 20px 20px 0;min-width:250px;\"><a href=\"http://heraldsun.local/wp-admin/admin.php?action=wpcom_thumbnail_edit&#038;id=44&#038;size=gallery_alt_image\" target=\"_blank\"><strong>gallery_alt_image</strong><br /><img src=\"http://heraldsun.local/wp-content/uploads/sites/10/2017/03/cropped-mars-1-288x433.jpg\" alt=\"gallery_alt_image\" /></a></div><div style=\"float:left;margin:0 20px 20px 0;min-width:250px;\"><a href=\"http://heraldsun.local/wp-admin/admin.php?action=wpcom_thumbnail_edit&#038;id=44&#038;size=tablet_portrait\" target=\"_blank\"><strong>tablet_portrait</strong><br /><img src=\"http://heraldsun.local/wp-content/uploads/sites/10/2017/03/cropped-mars-1.jpg\" alt=\"tablet_portrait\" /></a></div><div style=\"float:left;margin:0 20px 20px 0;min-width:250px;\"><a href=\"http://heraldsun.local/wp-admin/admin.php?action=wpcom_thumbnail_edit&#038;id=44&#038;size=tablet_square\" target=\"_blank\"><strong>tablet_square</strong><br /><img src=\"http://heraldsun.local/wp-content/uploads/sites/10/2017/03/cropped-mars-1.jpg\" alt=\"tablet_square\" /></a></div><div style=\"float:left;margin:0 20px 20px 0;min-width:250px;\"><a href=\"http://heraldsun.local/wp-admin/admin.php?action=wpcom_thumbnail_edit&#038;id=44&#038;size=tablet_wide\" target=\"_blank\"><strong>tablet_wide</strong><br /><img src=\"http://heraldsun.local/wp-content/uploads/sites/10/2017/03/cropped-mars-1.jpg\" alt=\"tablet_wide\" /></a></div><div style=\"float:left;margin:0 20px 20px 0;min-width:250px;\"><a href=\"http://heraldsun.local/wp-admin/admin.php?action=wpcom_thumbnail_edit&#038;id=44&#038;size=tablet_thumbnail\" target=\"_blank\"><strong>tablet_thumbnail</strong><br /><img src=\"http://heraldsun.local/wp-content/uploads/sites/10/2017/03/cropped-mars-1-196x110.jpg\" alt=\"tablet_thumbnail\" /></a></div><div style=\"float:left;margin:0 20px 20px 0;min-width:250px;\"><a href=\"http://heraldsun.local/wp-admin/admin.php?action=wpcom_thumbnail_edit&#038;id=44&#038;size=tablet_gallery_landscape\" target=\"_blank\"><strong>tablet_gallery_landscape</strong><br /><img src=\"http://heraldsun.local/wp-content/uploads/sites/10/2017/03/cropped-mars-1-512x488.jpg\" alt=\"tablet_gallery_landscape\" /></a></div><div style=\"float:left;margin:0 20px 20px 0;min-width:250px;\"><a href=\"http://heraldsun.local/wp-admin/admin.php?action=wpcom_thumbnail_edit&#038;id=44&#038;size=tablet_square_small\" target=\"_blank\"><strong>tablet_square_small</strong><br /><img src=\"http://heraldsun.local/wp-content/uploads/sites/10/2017/03/cropped-mars-1-234x234.jpg\" alt=\"tablet_square_small\" /></a></div></div></div></td>\n\t\t</tr>\n</table>",
    "meta": ""
  }
}

Last edited 8 years ago by aussieguy123 (previous) (diff)

#6 @westonruter
8 years ago

@aussieguy123 so actually you're not experiencing the issue on WordPress.com but rather on VIP Quickstart?

#7 @aussieguy123
8 years ago

@westonruter I've managed to reproduce this issue on both WordPress.com VIP and also our custom docker environment based on VIP quickstart.

#8 @aussieguy123
8 years ago

  • Keywords reporter-feedback removed

#9 @aussieguy123
8 years ago

A few of our non productions servers (also based on vip quickstart) got the issue as well

Version 0, edited 8 years ago by aussieguy123 (next)

#10 @westonruter
8 years ago

@aussieguy123 I suggest debugging in VIP Quickstart, specifically in this code:

<?php
// Disable automatic creation of intermediate images
add_filter( 'intermediate_image_sizes', 'wpcom_intermediate_sizes' );
function wpcom_intermediate_sizes ( $sizes ) {
    if ( ! defined( 'JETPACK_DEV_DEBUG' ) || ! JETPACK_DEV_DEBUG ) {
            return array();
    }
    return $sizes;
}

It's not clear to me if this intermediate_image_sizes filter here is used incorrectly, or if it is being used properly and there is a core bug, in regards to the site icon control.

#11 @aussieguy123
8 years ago

@westonruter I just did some debugging for this code on my local, after triggering the issue by uploading a site_icon.

Basically the wpcom_intermediate_sizes function takes the sizes and returns them, doesn't do any filtering. I don't think its this function causing the issue.

I still get the issue even if I temporarily delete that function and remove the filter.

Last edited 8 years ago by aussieguy123 (previous) (diff)

@westonruter
8 years ago

#12 @westonruter
8 years ago

@aussieguy123 I think 40010.1.diff is a better approach for fixing the symptom you're experiencing. Nevertheless, I am unsure as to why the full size would be undefined. Since this has only been reported in the context of WordPress.com I'd suggest looping in a VIP code wrangler to identify the root cause.

Can you try selectively activating Jetpack modules to identify which module specifically is causing the problem when the plugin is active? I created a Jetpack issue for you and I suggest you follow up there to identify the root cause with the Jetpack team: https://github.com/Automattic/jetpack/issues/6683

Once the root cause is identified, either a fix for Jetpack will ensue and we can close this ticket, or we'll confirm the need for committing a patch for this to core.

#13 @swissspidy
8 years ago

Previously: #36749.

#14 @trepmal
8 years ago

I can't tell if there's a reason the template uses data.attachment.sizes.full.url instead of data.attachment.url. Looking at https://core.trac.wordpress.org/browser/trunk/src/wp-includes/media.php#L3192, it appears that both will have the same value in this context.

I like attachment:40010.1.diff for the fallback, or even just using data.attachment.url in the first place.

Since it's trivial to remove the full-size, it would be nice if the assumption of its presence wasn't made.

#15 @aussieguy123
8 years ago

WordPress.com VIP found the trigger for this the issue.

We have a plugin which is removing the 'full' size via the 'wp_prepare_attachment_for_js' filter.

This is what triggers the bug. Either my patch or Weston's patch should fix the front end in cases like ours where the 'full' size is removed. I think one should be applied (VIP also suggested this in our support ticket), since its good practice to check if the size exists before using it.

Last edited 8 years ago by aussieguy123 (previous) (diff)

#16 @westonruter
8 years ago

  • Keywords has-patch added
  • Milestone changed from Awaiting Review to 4.7.4

@aussieguy123 Can you provide the source of the wp_prepare_attachment_for_js filter that is being used? I tried acking through vip-wpcom-mu-plugins but couldn't find it.

#17 @aussieguy123
8 years ago

@westonruter its a part of our new authoring plugin. Relevant functions are:

<?php
class Image_Crop_Handler {
        public $wp_image_sizes_to_remove = array( 'thumbnail', 'medium', 'large', 'full' );

        /**
         * Remove default WordPress image sizes.
         *
         * @param $attachment
         *
         * @filter wp_prepare_attachment_for_js
         *
         * @return mixed
         */
        public function remove_non_spp_image_sizes( $attachment ) {
                $attachment = $this->replace_full_size_label( $attachment );
                if ( empty( $attachment['sizes'] ) ) {
                        return $attachment;
                }

                foreach ( $attachment['sizes'] as $label => $attributes ) {
                        if ( ! $this->is_valid_spp_image_size( $label, $attributes ) ) {
                                unset( $attachment['sizes'][ $label ] );
                        }
                }

                return $attachment;
        }

        /**
         * Whether the image size is a valid SPP image size or not
         *
         * @param $label
         * @param $attributes
         *
         * @return bool
         */
        public function is_valid_spp_image_size( $label, $attributes ) {
                if ( in_array( $label, $this->wp_image_sizes_to_remove, true ) ) {
                        return false;
                }

                if ( isset( $this->image_sizes[ $label ] ) ) {
                        $height = $this->image_sizes[ $label ]['height'];
                        $width  = $this->image_sizes[ $label ]['width'];

                        if ( $height !== $attributes['height'] || $width !== $attributes['width'] ) {
                                return false;
                        }
                }

                return true;
        }
}

We have some standardized image sizes we use, which is why we remove the default WordPress sizes.

Last edited 8 years ago by aussieguy123 (previous) (diff)

#18 @westonruter
8 years ago

  • Summary changed from Regression for #36749. Uploading an image into site_icon breaks customizer to Customize: Template for site icon control fails to check if full image size exists before using
  • Version changed from trunk to 4.5

#19 @westonruter
8 years ago

  • Owner set to westonruter
  • Resolution set to fixed
  • Status changed from new to closed

In 40314:

Customize: Harden site_icon control template to account for when full image size is missing.

Props aussieguy123, westonruter.
See #36749.
Fixes #40010.

#20 @westonruter
8 years ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Re-opening for consideration in 4.7.4

#21 @swissspidy
8 years ago

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

In 40332:

Customize: Harden site_icon control template to account for when full image size is missing.

Props aussieguy123, westonruter.
See #36749.
Fixes #40010.

Merges [40314] to the 4.7 branch.

Note: See TracTickets for help on using tickets.