WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

Last modified 2 years ago

#29127 closed defect (bug) (fixed)

Bundled Themes: fix escaping and minor code style issues

Reported by: lancewillett Owned by:
Milestone: 4.2 Priority: normal
Severity: normal Version:
Component: Bundled Theme Keywords: has-patch
Focuses: Cc:
PR Number:

Description

Not urgent, but all bundled themes needed a look with PHP Code Sniffer and plugins like Theme Check and VIP Scanner to catch anything we've missed (or was introduced after the last thorough check).

Attachments (9)

29127.twentyfourteen.patch (7.2 KB) - added by lancewillett 5 years ago.
Twenty Fourteen cleanup
29127.twentythirteen.patch (3.4 KB) - added by lancewillett 5 years ago.
Twenty Thirteen cleanup
29127.twentytwelve.patch (5.1 KB) - added by lancewillett 5 years ago.
Twenty Twelve cleanup
29127.twentyeleven.patch (14.9 KB) - added by lancewillett 5 years ago.
Twenty Eleven cleanup: note — removes extract() from Twenty_Eleven_Ephemera_Widget code, see #22400
29127.twentyten.patch (13.1 KB) - added by lancewillett 5 years ago.
Twenty Ten cleanup; note adds two missing "Featured Image" related tags
29127.twentythirteen.2.patch (3.4 KB) - added by lancewillett 5 years ago.
Second pass at Twenty Thirteen cleanup
29127.twentytwelve.2.patch (5.3 KB) - added by lancewillett 5 years ago.
Second pass at Twenty Twelve cleanup
29127.twentyeleven.2.patch (14.7 KB) - added by lancewillett 5 years ago.
Second pass at Twenty Eleven cleanup
29127.twentyten.2.patch (12.3 KB) - added by lancewillett 5 years ago.
Second pass at Twenty Ten cleanup

Download all attachments as: .zip

Change History (26)

@lancewillett
5 years ago

Twenty Fourteen cleanup

@lancewillett
5 years ago

Twenty Thirteen cleanup

@lancewillett
5 years ago

Twenty Twelve cleanup

@lancewillett
5 years ago

Twenty Eleven cleanup: note — removes extract() from Twenty_Eleven_Ephemera_Widget code, see #22400

@lancewillett
5 years ago

Twenty Ten cleanup; note adds two missing "Featured Image" related tags

#1 @lancewillett
5 years ago

  • Keywords needs-testing added

#2 follow-up: @philiparthurmoore
5 years ago

For the WordPress Coding Standards sniffs, I've had to exclude WordPress.WhiteSpace.ScopeIndent.Incorrect because the themes and plugins I'm working on never pass them without serious code adjustments that I'm not comfortable making due to readability and such. If you end up using every Coding Standards rule and excluding nothing, can you note that? I want to make sure that I'm following the examples of the default themes as closely as possible but a few of these rulesets are very tedious without big payback. (In general I love them, though, and have been scanning everything with them.)

#3 in reply to: ↑ 2 @lancewillett
5 years ago

Replying to philiparthurmoore:

If you end up using every Coding Standards rule and excluding nothing, can you note that?

Yes, I added this to WordPress/ruleset.xml (might not be the best way to do it, but worked for me):

	<rule ref="WordPress.WhiteSpace.ScopeIndent">
		// Ignore wp-content/themes/ entirely for scope indent checks.
		<exclude-pattern>*/wp-content/themes/*</exclude-pattern>
	</rule>
Last edited 5 years ago by lancewillett (previous) (diff)

#4 follow-ups: @obenland
5 years ago

  • Milestone changed from 4.0 to Future Release

Twenty Fourteen patch looks good.

Twenty Thirteen patch:

  • Can we move the definition of $style to the top of the function?

Twenty Twelve patch:

  • In header.php: Adding esc_url() around header_image() will not work, as it echoes its content.
  • Can we move the definition of $style to the top of the function?
  • Let's keep the new line at end of file in inc/custom-header.php.

Twenty Eleven patch:

  • Do we allow HTML in the site description? If we do, using esc_html() around the site description might break things.
  • In header.php: Adding esc_url() around header_image() will not work, as it echoes its content.

Twenty Ten patch:

  • In category.php: Using esc_html() around the category description might break things.
  • If we allow HTML in the site description, using esc_html() around the site description might break things.
  • In header.php: Adding esc_url() around header_image() will not work, as it echoes its content.

#5 in reply to: ↑ 4 @SergeyBiryukov
5 years ago

Replying to obenland:

  • In header.php: Adding esc_url() around header_image() will not work, as it echoes its content.

It's also not needed, as header_image() is already escaped. See #23664.

@lancewillett
5 years ago

Second pass at Twenty Thirteen cleanup

@lancewillett
5 years ago

Second pass at Twenty Twelve cleanup

@lancewillett
5 years ago

Second pass at Twenty Eleven cleanup

@lancewillett
5 years ago

Second pass at Twenty Ten cleanup

#6 in reply to: ↑ 4 @lancewillett
5 years ago

Replying to obenland:

  • Can we move the definition of $style to the top of the function?
  • In header.php: Adding esc_url() around header_image() will not work, as it echoes its content.
  • In category.php: Using esc_html() around the category description might break things.

Fixed.

  • Do we allow HTML in the site description? If we do, using esc_html() around the site description might break things.

Fixed. We do not allow HTML, and it's escaped with esc_html() already: see https://core.trac.wordpress.org/browser/trunk/src/wp-includes/formatting.php#L3303

#7 @lancewillett
5 years ago

  • Keywords needs-refresh added
  • Milestone changed from Future Release to 4.2

#8 @lancewillett
5 years ago

Patches probably need testing and a refresh.

#9 @lancewillett
5 years ago

In 31250:

Twenty Fourteen: fix escaping and minor code style issues. See #29127.

#10 @lancewillett
5 years ago

In 31260:

Twenty Thirteen: fix escaping and minor code style issues. See #29127.

#11 @lancewillett
5 years ago

In 31261:

Twenty Twelve: fix escaping and minor code style issues. See #29127.

#12 @lancewillett
5 years ago

In 31262:

Twenty Twelve: more fixes for escaping and minor code style issues. See #29127.

#13 @lancewillett
5 years ago

In 31265:

Twenty Eleven: fix escaping and minor code style issues. See #29127.

#14 @lancewillett
5 years ago

In 31266:

Twenty Ten: fix escaping and minor code style issues. See #29127.

#15 @lancewillett
5 years ago

In 31267:

Twenty Eleven: fix indentation (spaces to tabs). See #29127.

#16 @lancewillett
5 years ago

  • Keywords needs-testing needs-refresh removed
  • Resolution set to fixed
  • Status changed from new to closed

Open new tickets for new issues.

This ticket was mentioned in Slack in #themereview by dingo_d. View the logs.


2 years ago

Note: See TracTickets for help on using tickets.