WordPress.org

Make WordPress Core

Opened 16 months ago

Closed 14 months ago

Last modified 5 months ago

#36749 closed defect (bug) (fixed)

Customizer wont load: issue with site-icon control

Reported by: raxcido Owned by: 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 15 months ago.
36749.2.diff (707 bytes) - added by neverything 15 months ago.
36749.3.diff (691 bytes) - added by swissspidy 15 months ago.
36749.4.diff (2.0 KB) - added by ocean90 15 months ago.

Download all attachments as: .zip

Change History (26)

#1 @Fabio A
16 months 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
16 months ago

#36820 was marked as a duplicate.

#3 @ocean90
16 months ago

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

Moving for review.

#4 follow-up: @swissspidy
16 months 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
16 months 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
15 months ago

#6 @neverything
15 months ago

  • Keywords has-patch added; needs-patch removed

Added patches for both variations.

@swissspidy
15 months ago

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


15 months ago

#9 @swissspidy
15 months 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
15 months ago

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

#11 @ocean90
15 months 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
15 months ago

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


15 months ago

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


15 months ago

#14 @swissspidy
15 months ago

#37030 was marked as a duplicate.

#15 @swissspidy
15 months 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
15 months 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
15 months 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
14 months ago

  • Keywords commit added

+1 for implementing 36749.4.diff then.

#19 @ocean90
14 months 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
14 months 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
5 months 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
5 months 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.