Make WordPress Core

Opened 3 years ago

Closed 2 years ago

#53370 closed enhancement (fixed)

Theme thumbnail/screenshot does not contain version string

Reported by: codente's profile codente Owned by: audrasjb's profile audrasjb
Milestone: 6.0 Priority: normal
Severity: minor Version: 5.7.2
Component: Themes Keywords:
Focuses: Cc:

Description

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

Attachments (2)

theme-screenshot-version.patch (1.6 KB) - added by codente 3 years ago.
53370.1.diff (1.7 KB) - added by audrasjb 2 years ago.
Themes: Add version to theme screenshot URL for better browser cache refreshing

Download all attachments as: .zip

Change History (23)

#1 @desrosj
3 years ago

  • Milestone changed from Awaiting Review to 5.9

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.

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


2 years ago

@audrasjb
2 years ago

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

#3 @audrasjb
2 years ago

Patch refreshed against trunk

#4 @chaion07
2 years ago

  • Keywords commit added

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

#5 @SergeyBiryukov
2 years ago

  • Keywords commit removed

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.

Last edited 2 years ago by SergeyBiryukov (previous) (diff)

#6 @hellofromTonya
2 years ago

  • Milestone changed from 5.9 to Future Release

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.

#7 @SergeyBiryukov
2 years ago

  • Milestone changed from Future Release to 6.0

#8 @davidbaumwald
2 years ago

  • Keywords needs-refresh added

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

This ticket was mentioned in PR #2424 on WordPress/wordpress-develop by audrasjb.


2 years ago
#9

  • Keywords needs-refresh removed

#10 @audrasjb
2 years ago

Patch refreshed via the above PR

#11 @audrasjb
2 years ago

  • Owner set to audrasjb
  • Status changed from new to accepted

Self assigning for final review and commit.

#12 @audrasjb
2 years ago

  • Keywords commit added

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.

#13 @audrasjb
2 years ago

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

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.

#15 @audrasjb
2 years ago

  • Keywords needs-patch added; has-patch commit removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

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

This ticket was mentioned in PR #2428 on WordPress/wordpress-develop by audrasjb.


2 years ago
#16

  • Keywords has-patch added; needs-patch removed

#17 @audrasjb
2 years ago

In 52948:

Themes: Add version to theme screenshot URL in WP_Themes_List_Table.

Follow-up to [52947].

See #53370.

#19 @SergeyBiryukov
2 years ago

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.

#20 @peterwilsoncc
2 years ago

  • Keywords has-patch removed

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.

#21 @audrasjb
2 years ago

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

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.

Note: See TracTickets for help on using tickets.