Customizer: use attachment ID to show current header

[sirbrillig]

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.

[sirbrillig]

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

[azaozz]

Yesterday got another report of this breaking.

[ocean90]

@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.

[ocean90]

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

[sirbrillig]

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.

[sirbrillig]

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. :-)

[sirbrillig]

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

[sirbrillig]

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.

[sirbrillig]

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

[sirbrillig]

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

[ocean90]

@sirbrillig Does this ticket depend on #31786?

[sirbrillig]

@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).

[ocean90]

@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()?

[sirbrillig]

Moved fallback code to api.HeaderControl.ready

[sirbrillig]

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.

[sirbrillig]

Forgot to include attachment_id

[ocean90]

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.