WordPress.org

Make WordPress Core

#21749 closed defect (bug) (fixed)

Wrong URL for theme screenshot

Reported by: pavelevap Owned by:
Milestone: 3.4.2 Priority: normal
Severity: normal Version: 3.4.1
Component: Themes Keywords: has-patch
Focuses: Cc:

Description

Theme directory sometimes contains "space", I saw for example directory "Tuscan 1.0" etc. But in this case, screenshot in not shown on theme list (there is only icon of missing picture), because URL is transformed and space stripped:

/wp-content/themes/Tuscan1.0/screenshot.png

I understand that it is not best practice, but it is regression (in WP 3.3.x it worked well).

Also, it would be better to show some simple default picture for themes without screenshot (for example "This theme has not screenshot yet") instead of grey place...

Attachments (4)

21749.patch (713 bytes) - added by SergeyBiryukov 20 months ago.
21749.2.patch (1.6 KB) - added by SergeyBiryukov 20 months ago.
21749.3.patch (1.6 KB) - added by SergeyBiryukov 20 months ago.
21749.diff (10.7 KB) - added by nacin 20 months ago.

Download all attachments as: .zip

Change History (21)

comment:1 SergeyBiryukov20 months ago

  • Milestone changed from Awaiting Review to 3.4.2

SergeyBiryukov20 months ago

SergeyBiryukov20 months ago

comment:2 SergeyBiryukov20 months ago

  • Keywords has-patch added

The space is stripped by esc_url(), since the space is not encoded:
http://core.trac.wordpress.org/browser/tags/3.4.1/wp-admin/includes/class-wp-themes-list-table.php#L156

In 3.3, the screenshot was output directly:
http://core.trac.wordpress.org/browser/tags/3.3.3/wp-admin/includes/class-wp-themes-list-table.php#L160

I guess get_*_directory_uri() should return encoded results.

comment:3 SergeyBiryukov20 months ago

  • Keywords has-patch removed

21749.2.patch would break if the theme is one directory deeper:

wp-content/themes/Tuscan 1.0/Tuscan

comment:4 pavelevap20 months ago

There is also another theme example which does not work in latest trunk (in deeper directory):

wp-content/themes/Some_Theme/Theme 1.0

SergeyBiryukov20 months ago

comment:5 SergeyBiryukov20 months ago

  • Keywords has-patch added

comment:6 nacin20 months ago

get_stylesheet_directory_uri() and get_template_directory_uri() have always been this way. I want to think about what could break if we change that now. On the other hand, we should be able to make some changes to WP_Theme.

21749.3.patch is a good start, though Live Preview and Preview links remain broken. We can fix that with a change in the list table and in wp_customize_url(). It looks like the preview output buffer on the frontend still fails due to spaces not being allowed in the regex in preview_theme(). That's a long-standing bug, though.

Looking to do some unit tests for this, then run with it.

comment:8 nacin20 months ago

In [21712]:

URL encode the stylesheet directory values passed to WP_Theme's get_stylesheet_directory_uri() and get_template_directory_uri(). props SergeyBiryukov, see #21749.

comment:9 nacin20 months ago

In [21713]:

URL encode the theme stylesheet passed into wp_customize_url(). see #21749.

nacin20 months ago

comment:10 nacin20 months ago

21749.diff is the result of an audit of 'stylesheet' or 'template' used as query args. (Might have missed something, but got all the obvious ones.)

Some of these are actually okay, such as whenever it is passed into wp_nonce_url(), since wp_nonce_url() uses add_query_arg(), which ensures existing query args are encoded before appending new ones.

comment:11 nacin20 months ago

In [21714]:

URL encode the stylesheet directory values passed to WP_Theme's get_stylesheet_directory_uri() and get_template_directory_uri(). props SergeyBiryukov, see #21749. for the 3.4 branch.

comment:12 nacin20 months ago

Reviewed these again.

  • ms-themes list table is almost exclusively sending an unencoded stylesheet through wp_nonce_url(), which will get it encoded. The only exception is wp-admin/themes.php, which happens to be a regression.
  • The old-school preview links in class-wp-upgrader need urlencoded stylesheet values. This is a regression, though the Live Preview (fixed by [21713]) replaces these links in most situations.
  • The network enable link in class-wp-upgrader goes through wp_nonce_url(), which will encode it, so it is not broken.
  • In wp-admin/includes/theme.php (delete_theme()), this goes through wp_nonce_url(), so it is not broken.
  • The decoding in theme-editor.php is not intentional and should be removed, but does not break anything.

For 3.4: Merge [21713] to the branch. [21712] is already merged, see [21714]. Fix links in the upgrader. Ignore the "Edit" link in ms-themes.

For 3.5: Make sure all of 21749.diff makes it in, rather than relying on wp_nonce_url() to do the work.

comment:13 nacin20 months ago

In [21752]:

Fix old-school Preview links when a theme directory contains spaces. Fix the theme-editor.php link from MS themes screens when a theme directory contains spaces. see #21749.

comment:14 nacin20 months ago

In [21753]:

URL encode the theme stylesheet passed into wp_customize_url(). see #21749. Merges [21713] to the 3.4 branch.

comment:15 nacin20 months ago

In [21754]:

Fix old-school Preview links when a theme directory contains spaces. Fix the theme-editor.php link from MS themes screens when a theme directory contains spaces. see #21749. Merges [21752] to the 3.4 branch.

comment:16 nacin20 months ago

In [21755]:

Always URL-encode a stylesheet directory value before using it in a URL. These situations are saved by wp_nonce_url(), but we should not depend on that. see #21749, for trunk only.

comment:17 nacin20 months ago

  • Resolution set to fixed
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.