Theme thumbnail/screenshot does not contain version string

[codente]

As a result, if you change it, it's cached by the browser. I propose adding a version string to make sure it's updated if you update the image

[desrosj]

Thanks for this one, @codente! This makes sense to me.

The window to include it in 5.8 has passed, so moving to 5.9.

[audrasjb]

Themes: Add version to theme screenshot URL for better browser cache refreshing

[audrasjb]

Patch refreshed against trunk

[chaion07]

Hello @codente! Thank you for reporting this. This ticket was discussed during a recent ​Bug-scrub session. Based on the feedback received we think its ready for commit. The patch was also refreshed. Thanks!

Props to @audrasjb & @costdev

[SergeyBiryukov]

Hi there, thanks for the ticket and the patch!

This looks good to me. However, if you search for theme-screenshot in core, you'll find a few more instances where theme screenshots are displayed. For consistency, I think this should be done for all of them, so the patch needs some more work. Removing the commit tag for now.

[hellofromTonya]

As of yesterday, 5.9 is now in Feature Freeze. Moving this ticket to the next release cycle. But 6.0 is not yet available for selection. Moving it to Future Release. Once 6.0 is available, please feel free to move it into that milestone.

[davidbaumwald]

Marking for a refresh, per Sergey's comment, to hopefully gain some attention during this cycle.

Trac ticket: https://core.trac.wordpress.org/ticket/53370

[audrasjb]

Patch refreshed via the above PR

[audrasjb]

Self assigning for final review and commit.

[audrasjb]

I tested the last PR and it successfully adds the version string in each screenshot, on both WP-Admin and the Customizer.

Marking as good for commit.

[audrasjb]

In 52947:

Themes: Add version to theme screenshot URL for better browser cache handling.

This change appends the theme version number to the URL of screenshots that appear in various place of the WordPress Admin. As a result, browsers will be able to refresh the screenshot as needed when the theme is updated.

Props codente, desrosj, audrasjb, SergeyBiryukov.
Fixes #53370.

Committed in https://core.trac.wordpress.org/changeset/52947

[audrasjb]

I spotted at least one missed file. Reopening for follow-up fixes.

Trac ticket: https://core.trac.wordpress.org/ticket/53370

[audrasjb]

In 52948:

Themes: Add version to theme screenshot URL in WP_Themes_List_Table.

Follow-up to [52947].

See #53370.

Committed in https://core.trac.wordpress.org/changeset/52948

[SergeyBiryukov]

In 52949:

Themes: Use esc_url() for theme screenshots on the Themes screen.

This brings consistency with how screenshots are escaped elsewhere.

Follow-up to [52020], [52947].

See #53370.

[peterwilsoncc]

Do additional items need a version string or is this ticket ready to close?

The only additional location I could find was the theme REST endpoint.

​https://github.com/WordPress/wordpress-develop/blob/8db1549c0f1af05fbb919ad2eeb7f7bff5a63da9/src/wp-includes/rest-api/endpoints/class-wp-rest-themes-controller.php#L265-L268

If the version string is required everywhere, then including the hash in a backward compatible fashion in WP_Themes::get_screenshot() seems a more sensible approach.

Each of the items has-patch applies to has been committed, so I am removing the keyword to reflect any future work that might be required.

[audrasjb]

I think we can close this as fixed, the REST endpoint should be implemented on a dedicated ticket, since we need to make sure it's backward compatible and also plugin icons would probably deserve the same treatment.