WordPress.org

Make WordPress Core

Opened 12 months ago

Closed 12 months ago

Last modified 4 days ago

#39152 closed defect (bug) (fixed)

Twenty Seventeen: phpcs errors and warnings

Reported by: dingo_bastard Owned by: davidakennedy
Milestone: 4.8 Priority: normal
Severity: normal Version: 4.7
Component: Bundled Theme Keywords: has-patch
Focuses: coding-standards Cc:

Description

Since we are aiming towards automation of the theme review, as described in this blog post I wanted to check what's the status with the newly Twenty Seventeen theme.

I used the WordPress coding standard (which also includes the VIP standard sniffs which we can ignore), and ignored the *.js and *.css files (errors there were mostly based about spacing and inline comments, which the phpcs pulled from php rules).

I'm attaching the log file with the errors. Some can be easily fixed by running phpcbf, but some should be taken care of manually - for instance in the content-video.php file located in the \template-parts\post folder on line 67 there is

echo $video_html;

which should be escaped (I added esc_html() and the test videos provided in the theme unit test worked).

Hope this helps :)

Attachments (5)

twseventeen.txt (29.4 KB) - added by dingo_bastard 12 months ago.
phpcs error log of the Twenty Seventeen theme
39152.diff (35.4 KB) - added by sstoqnov 12 months ago.
Fix php errors and warnings in twentyseventeen
39152.2.diff (22.6 KB) - added by sstoqnov 12 months ago.
39152.3.diff (22.6 KB) - added by sstoqnov 12 months ago.
Remove extra space in content-video.php
39152.4.patch (22.7 KB) - added by davidakennedy 12 months ago.
Added space after file comments in some files for consistency.

Download all attachments as: .zip

Change History (13)

@dingo_bastard
12 months ago

phpcs error log of the Twenty Seventeen theme

#1 @davidakennedy
12 months ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 4.8
  • Type changed from enhancement to defect (bug)

Thanks for the ticket @dingo_bastard!

Most of these seem minor (spacing related), and many are false positives related to escaping HTML that doesn't need escaped. But the spacing issues and code commenting should be addressed where it's reasonable to do so.

@sstoqnov
12 months ago

Fix php errors and warnings in twentyseventeen

#2 @sstoqnov
12 months ago

  • Keywords has-patch added; needs-patch removed

In 39152.diff I fix most of the issues.
Some of escaping warnings were skipped.

#3 @davidakennedy
12 months ago

Hey @sstoqnov! Thanks for the patch – it's looking like a very nice first pass. Here are a few items of feedback:

  • In custom-header.php, @param array $settings Video settings, there's an extra space.
  • In customizer.php, the new parameter comments should end in a full stop (period).
  • In content-audio.php, that file should end in a new line.
  • In functions.php, @param string $link Link to single post/page, there's an extra space. It must also end in a full stop (period).

The other larger change I see in a few spots is indenting code after a if ( ! function_exists() call. You changed it a few spots, but not all of them. I realize that it's a code standard to indent after one. Historically, I don't think it's been done in some functions.php files in default themes because it's possible that the indention could get very deep. I'd rather be consistent here and not do any of them. Or all of them. Personally, I rather just leave them.

Thanks again for the patch! Looking forward to the next iteration.

@sstoqnov
12 months ago

@sstoqnov
12 months ago

Remove extra space in content-video.php

#4 @sstoqnov
12 months ago

Hey @davidakennedy, thank you for your comment.
I have addressed the issues.

Regarding the if ( ! function_exists() call I don't think we should remove the indention, just to keep the functions indention low. I mean that if the function is written right it will not go very deep.

I have searched for this check in twentyseventeen and all of them were fixed.
However I have remove the indention to keep the code consistent, but I will be happy if we add the indention to all themes.
What do you think, is it ok to do that? And should I open a new ticket or can continue here?

#5 @davidakennedy
12 months ago

Thanks for the refreshed patch @sstoqnov! It looks good from a quick glance.

However I have remove the indention to keep the code consistent, but I will be happy if we add the indention to all themes.
What do you think, is it ok to do that? And should I open a new ticket or can continue here?

I don't think that's necessary here. I'd opt to just leave everything the way it is now. Changing the indention makes sense because it’s a code standard, but the difference in readability isn't big enough to bother with it here.

@davidakennedy
12 months ago

Added space after file comments in some files for consistency.

#6 @davidakennedy
12 months ago

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

In 39618:

Twenty Seventeen: Improves code readability and code standards in files

Adds better DocBlock comments and fixes some spacing issues based on PHP_CodeSniffer WordPress coding standards.

Props sstoqnov.

Fixes #39152.

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


5 months ago

#8 @GaryJ
4 days ago

  • Focuses coding-standards added
Note: See TracTickets for help on using tickets.