Twenty Seventeen: phpcs errors and warnings

[dingo_d]

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 :)

[dingo_bastard]

phpcs error log of the Twenty Seventeen theme

[davidakennedy]

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]

Fix php errors and warnings in twentyseventeen

[sstoqnov]

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

[davidakennedy]

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]

Remove extra space in content-video.php

[sstoqnov]

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?

[davidakennedy]

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]

Added space after file comments in some files for consistency.

[davidakennedy]

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.