WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 4 years ago

#31742 closed defect (bug) (fixed)

Customizer: use attachment ID to show current header

Reported by: sirbrillig Owned by: ocean90
Milestone: 4.2 Priority: normal
Severity: normal Version: 3.9
Component: Customize Keywords: has-patch
Focuses: Cc:

Description

api.HeaderTool.ChoiceView sets the current header image model (api.HeaderTool.currentHeader) by comparing the current image URL (api.get().header_image) to the model URL for each recently updated header image. This relies on the URLs being identical, which may not be the case if, for example, the value of api.get().header_image uses https and the value of the model's URL uses http (which is the case on WordPress.com).

I think it would be better to use the attachment ID to find the current header image. The attached patch modifies the check in api.HeaderTool.ChoiceView to compare the current image attachment ID (api.get().header_image_data.attachment_id) with the model attachment ID. Because sometimes the current header may not have an ID (eg: if it is set to random-uploaded-image), this also falls back to comparing the choice attribute.

Attachments (7)

31742.diff (1.0 KB) - added by sirbrillig 5 years ago.
31742.1.diff (7.2 KB) - added by sirbrillig 5 years ago.
Same as previous patch, but with minified javascript file included.
31742.2.diff (1.1 KB) - added by sirbrillig 5 years ago.
31742.3.diff (640 bytes) - added by sirbrillig 4 years ago.
A different approach: use a fallback for the current image.
31742.4.diff (2.3 KB) - added by sirbrillig 4 years ago.
Updated version of the previous patch, including numerous fixes and cleaned-up code.
31742.5.diff (1.7 KB) - added by sirbrillig 4 years ago.
Moved fallback code to api.HeaderControl.ready
31742.6.diff (1.8 KB) - added by sirbrillig 4 years ago.
Forgot to include attachment_id

Download all attachments as: .zip

Change History (24)

@sirbrillig
5 years ago

@sirbrillig
5 years ago

Same as previous patch, but with minified javascript file included.

#1 @mattwiebe
5 years ago

  • Keywords has-patch added

#2 @azaozz
5 years ago

  • Milestone changed from Awaiting Review to 4.2

Yesterday got another report of this breaking.

#3 @ocean90
5 years ago

  • Version changed from trunk to 3.9

@sirbrillig There is no need to patch minified files. This is done by our build tasks.
You should use develop.svn.wordpress.org for future patches.

I think we should check for the URL too, after ID and choice.

#4 @ocean90
5 years ago

  • Keywords needs-refresh added

The URL check must remain otherwise the default image which can be registered via default-image won't be selected.

@sirbrillig
5 years ago

#5 @sirbrillig
5 years ago

  • Keywords needs-refresh removed

There is no need to patch minified files.

Now I know. :-)

The URL check must remain otherwise the default image which can be registered via default-image won't be selected.

Good point! Tested on twentythirteen with the default image and this new patch successfully handles that.

#6 @sirbrillig
5 years ago

I also just realized that this does not solve the issue of default images if the protocol or domain are different, and that presents something of a challenge.

For example, on WordPress.com we use a CDN URL for the theme's default image (eg: https://s0.wp.com/wp-content/themes/pub/twentythirteen/images/headers/circle.png) which is on a different domain than the customizer is running on.

In that case, these patches (and the code that's currently in trunk, for that matter) will still show "No image set" when the default image is set. I'm not sure the best way to handle that case. I have a patch that will deal with it by stripping the protocol and domain name from the image paths when comparing them, but I don't really think that's the right way.

...but that might be a separate issue to deal with. :-)

Last edited 5 years ago by sirbrillig (previous) (diff)

This ticket was mentioned in Slack in #core-customize by mattwiebe. View the logs.


5 years ago

#8 @ocean90
5 years ago

  • Owner set to ocean90
  • Status changed from new to reviewing

@sirbrillig
4 years ago

A different approach: use a fallback for the current image.

#9 @sirbrillig
4 years ago

After some testing with the last patch, it seems there are still cases where it could be incorrect; in fact, sometimes it seems that it could be more incorrect than the existing behavior. I still think it would be a good idea to match without URL, but in this case we went back to the original issue that prompted this ticket, which is that in some cases the Current Image control reads "no image set" when in fact an image is actually set.

Taking another approach for that particular behavior, I made a new patch which simply checks, when displaying the current image, if its model data is inconsistent with the settings. If so, it falls back to using the current image from the settings.

More specifically, if api.get().header_image is set, and this.model.get('choice') is unset, then something is wrong and we might as well use api.get().header_image as the data.

#10 @sirbrillig
4 years ago

And for reference, the reason that the current header image might not be available is in #31786.

@sirbrillig
4 years ago

Updated version of the previous patch, including numerous fixes and cleaned-up code.

#11 @ocean90
4 years ago

@sirbrillig Does this ticket depend on #31786?

#12 @sirbrillig
4 years ago

@ocean90 No, in fact if #31786 is resolved, the issue this patch solves probably won't manifest. However, I think it may still be a good idea since it would protect against any other unforeseen cases which may cause the image not to be shown (eg: the http/https issue, which we fixed on wpcom with a filter on the theme_mod).

#13 @ocean90
4 years ago

@sirbrillig Thanks. I applied the patch and couldn't find an issue with it. But I also couldn't find a case where it fixes something. :-) How can i trigger the http/https issue for example?

Is the view actually the right place for this logic? What about api.HeaderControl.ready()?

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


4 years ago

@sirbrillig
4 years ago

Moved fallback code to api.HeaderControl.ready

#15 @sirbrillig
4 years ago

To simulate a case where this patch would prevent an issue, we can use a similar technique as reproducing #31786:

Using the Customizer's Header Image Control, upload two images with the same name on two different months (since images are organized by YYYY/MM) and then open the Customizer's Header Image Control. You would expect to see both images, but only one will be shown in the list of uploaded images below.

For example, if one image has the URL 2015/02/my-image.png and a second has the URL 2015/03/my-image.png only the second image will be shown.

Then we just need to set the current image to the uploaded image which does not show up in the list. My technique to do this is contrived, but a real-world situation might be if the file system backend caused the URL of the current image not to match the URL of the image in the uploaded list (as was previously the case on WP.com).

To fake this, apply the patch in #31786 and reload the Customizer. You'll see the previously hidden image is now visible in the list. Select it and save the Customizer. Then remove the patch from #31786 so the image is hidden again.

When the Customizer is reloaded, it will read "No image set", when in fact an image is set that just isn't visible in the list below it.

Applying 31742.5.diff in that situation will cause the fallback to fire and the correct image will be shown.

This ticket was mentioned in Slack in #core-customize by sirbrillig. View the logs.


4 years ago

@sirbrillig
4 years ago

Forgot to include attachment_id

#17 @ocean90
4 years ago

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

In 32091:

Customize Headers: Improve handling of the initial header model.

Headers are currently keyed by the basename of the image, see #31786. When two images have the same key only one image will be listed and the current image can be empty in the header control.
To prevent this we now fall back to the raw current header image if the image isn't in _wpCustomizeHeader.uploads.

props sirbrillig.
fixes #31742.

Note: See TracTickets for help on using tickets.