#36749 closed defect (bug) (fixed)
Customizer wont load: issue with site-icon control
Reported by: |
|
Owned by: |
|
---|---|---|---|
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)
Change History (26)
#3
@
9 years ago
- Milestone changed from Awaiting Review to 4.5.3
- Version changed from trunk to 4.5
Moving for review.
#4
follow-up:
↓ 5
@
9 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:
- The attachment is not an image
- 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
@
9 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?
#7
@
9 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.
9 years ago
#9
@
9 years ago
- Owner set to swissspidy
- Resolution set to fixed
- Status changed from new to closed
In 37456:
#10
@
9 years ago
- Keywords fixed-major added
- Resolution fixed deleted
- Status changed from closed to reopened
#11
@
9 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.
This ticket was mentioned in Slack in #core by adamsilverstein. View the logs.
9 years ago
This ticket was mentioned in Slack in #core by adamsilverstein. View the logs.
9 years ago
#15
@
9 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
@
9 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
@
9 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.
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.