WordPress.org

Make WordPress Core

Opened 9 years ago

Closed 9 years ago

Last modified 16 months ago

#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 9 years ago.
21749.2.patch (1.6 KB) - added by SergeyBiryukov 9 years ago.
21749.3.patch (1.6 KB) - added by SergeyBiryukov 9 years ago.
21749.diff (10.7 KB) - added by nacin 9 years ago.

Download all attachments as: .zip

Change History (22)

#1 @SergeyBiryukov
9 years ago

  • Milestone changed from Awaiting Review to 3.4.2

#2 @SergeyBiryukov
9 years 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.

#3 @SergeyBiryukov
9 years ago

  • Keywords has-patch removed

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

wp-content/themes/Tuscan 1.0/Tuscan

#4 @pavelevap
9 years 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

#5 @SergeyBiryukov
9 years ago

  • Keywords has-patch added

#6 @nacin
9 years 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.

#8 @nacin
9 years 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.

#9 @nacin
9 years ago

In [21713]:

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

@nacin
9 years ago

#10 @nacin
9 years 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.

#11 @nacin
9 years 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.

#12 @nacin
9 years 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.

#13 @nacin
9 years 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.

#14 @nacin
9 years ago

In [21753]:

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

#15 @nacin
9 years 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.

#16 @nacin
9 years 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.

#17 @nacin
9 years ago

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

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


16 months ago

Note: See TracTickets for help on using tickets.