Make WordPress Core

Opened 8 years ago

Closed 8 years ago

Last modified 7 years ago

#36749 closed defect (bug) (fixed)

Customizer wont load: issue with site-icon control

Reported by: raxcido's profile raxcido Owned by: swissspidy's profile swissspidy
Milestone: 4.5.3 Priority: normal
Severity: normal Version: 4.5
Component: Customize Keywords: has-patch commit
Focuses: javascript Cc:

Description

When i go to the customizer i get this error on console and after that Customizer can not load:

Uncaught TypeError: Cannot read property 'full' of undefined

There is not any plugin installed and wordpress is fresh, i don't know how this error occurred but it happened!
I traced this error and found data.attachment.sizes is undefined and it is in WP_Customize_Site_Icon_Control!

Please check line No. 62 of wp-includes/customize/class-wp-customize-site-icon-control.php

<# if ( data.attachment && data.attachment.id ) { #>

I think it should be:

<# if ( data.attachment && data.attachment.id && data.attachment.sizes) { #>

Attachments (4)

36749.diff (698 bytes) - added by neverything 8 years ago.
36749.2.diff (707 bytes) - added by neverything 8 years ago.
36749.3.diff (691 bytes) - added by swissspidy 8 years ago.
36749.4.diff (2.0 KB) - added by ocean90 8 years ago.

Download all attachments as: .zip

Change History (26)

#1 @Fabio A
8 years ago

I just now noticed this other bug report, I just reported the same here. I think my proposed fix works better, since it still makes the already existing favicon show up, yours would make wordpress think there's no favicon at all.

#2 @ocean90
8 years ago

#36820 was marked as a duplicate.

#3 @ocean90
8 years ago

  • Milestone changed from Awaiting Review to 4.5.3
  • Version changed from trunk to 4.5

Moving for review.

#4 follow-up: @swissspidy
8 years ago

  • Keywords needs-patch needs-testing added

So currently it looks like this:

<# if ( data.attachment && data.attachment.id ) { #>

Proposed fix 1:

<# if ( data.attachment && data.attachment.id && data.attachment.sizes) { #>

Proposed fix 2 from #36820:

<# data.attachment.sizes = data.attachment.sizes || { 'full': { 'url': data.attachment.url } } #>
<# if ( data.attachment && data.attachment.id ) { #>

Though I like the former approach more, I could not reproduce this issue on a fresh 4.5 site at all.

What would cause an image to not have a sizes property?

According to wp_prepare_attachment_for_js() this would happen in two cases:

  1. The attachment is not an image
  2. The attachment has no meta data, i.e wp_get_attachment_metadata() returns false

I can't add non-image attachments as site icons though…

#5 in reply to: ↑ 4 @Fabio A
8 years ago

Replying to swissspidy:

Proposed fix 2 from #36820:

<# data.attachment.sizes = data.attachment.sizes || { 'full': { 'url': data.attachment.url } } #>
<# if ( data.attachment && data.attachment.id ) { #>

Shouldn't that rather be

<# if ( data.attachment && data.attachment.id ) { #>
<# data.attachment.sizes = data.attachment.sizes || { 'full': { 'url': data.attachment.url } } #>

Or else, the code is bound to fail again if there's no attachment (and hence no data.attachment property), isn't it?

What would cause an image to not have a sizes property?

The favicon causing the issue was an old one, could it be that in the previous releases favicons were handled differently than they are now? As I said in #36820, reuploading the favicon automatically generated the requested data that was missing.

Alternatively, could this be a case of the database gone havoc due to some other bugs?

@neverything
8 years ago

@neverything
8 years ago

#6 @neverything
8 years ago

  • Keywords has-patch added; needs-patch removed

Added patches for both variations.

@swissspidy
8 years ago

#7 @swissspidy
8 years ago

  • Keywords commit added; needs-testing removed

That's quite similar to #36838, where meta sizes are checked as well. That's why I'm in favour of method A as in 36749.3.diff (36749.diff with fixed spacing).

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


8 years ago

#9 @swissspidy
8 years ago

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

In 37456:

Customize: Change attachment condition in the site icon control to prevent errors.

Props neverything.
Fixes #36749 for trunk.

#10 @swissspidy
8 years ago

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

#11 @ocean90
8 years ago

  • Keywords commit fixed-major removed

[37456] isn't a good idea because it fall backs to the "No image selected" box although an image is selected. I think we should only hide the preview section, see 36749.4.diff.

36749.2.diff looks also good.

@ocean90
8 years ago

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


8 years ago

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


8 years ago

#14 @swissspidy
8 years ago

#37030 was marked as a duplicate.

#15 @swissspidy
8 years ago

I assume the whole issue came to light because of the bug reported in #36534, which has caused many uploaded images not having intermediate sizes.

#37030 mentions other cases where the exact same fix as in [37456] was suggested because that's used elsewhere in core as well, like WP_Customize_Media_Control and wp-includes/media-template.php. If we're going to implement something like 36749.2.diff or 36749.4.diff, we should have a look at those files as well in a new ticket.

Since both patches lead to the same result, I tend to implement 36749.2.diff as it results in a cleaner diff.

#16 @celloexpressions
8 years ago

I also prefer the cleanliness of 36749.2.diff, although it needs an additional tab at the beginning of the added line.

Are we sure that this can only affect the site icon control? I think we should also add that line to WP_Customize_Media_Control to be safe, and for consistency.

#17 @ocean90
8 years ago

The decision shouldn't be made based on the "cleanliness" of a patch...

36749.4.diff just implements what WP_Customize_Media_Control is currently doing, separating the preview from the actions.

#18 @swissspidy
8 years ago

  • Keywords commit added

+1 for implementing 36749.4.diff then.

#19 @ocean90
8 years ago

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

In 37724:

Customize: Separate preview and actions in the site icon control.

Reverts [37456] to allow users to remove/change a site icon even if the attachment has corrupt/missing data about sizes.

Fixes #36749.

#20 @ocean90
8 years ago

In 37725:

Customize: Separate preview and actions in the site icon control.

Reverts [37456] to allow users to remove/change a site icon even if the attachment has corrupt/missing data about sizes.

Merge of [37724] to the 4.5 branch.

See #36749.

#21 @westonruter
7 years ago

In 40314:

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

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

#22 @swissspidy
7 years ago

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.