-
-
Notifications
You must be signed in to change notification settings - Fork 488
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add documentation for WordPress.WP.AlternativeFunctions.xml #2496
base: develop
Are you sure you want to change the base?
Add documentation for WordPress.WP.AlternativeFunctions.xml #2496
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution, and congratulations on your first WPCS PR, @pamprn09! I'm not a project maintainer, so take my review with a grain of salt.
I started reviewing this PR, and left some comments, but then started questioning if I was checking the right sniff since the code examples don't match what I expected. Could you please check before I continue (more information in one of the inline comments)?
@@ -0,0 +1,95 @@ | |||
<?xml version="1.0"?> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The file name and its location are incorrect. There is more information about it in #1722 and https://github.com/PHPCSStandards/PHP_CodeSniffer/blob/master/.github/CONTRIBUTING.md#writing-sniff-documentation.
The command below should display the documentation, but it doesn't because of that:
vendor/bin/phpcs --generator=Text --standard=WordPress --sniffs=WordPress.WP.AlternativeFunctions
The correct path should be WordPress/Docs/WP/AlternativeFunctionsStandard.xml
<![CDATA[ | ||
_e( 'Hello, World!', 'text-domain' ); | ||
$greeting = __( 'Hello, World!', 'text-domain' ); | ||
]]> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<![CDATA[ | |
_e( 'Hello, World!', 'text-domain' ); | |
$greeting = __( 'Hello, World!', 'text-domain' ); | |
]]> | |
<![CDATA[ | |
_e( 'Hello, World!', 'text-domain' ); | |
$greeting = __( 'Hello, World!', 'text-domain' ); | |
]]> |
The indentation for code samples should start at the beginning of the line. Otherwise, code samples with multiple lines will have incorrect indentation.
Also, per the #1722 description, each line within the code sample should be a maximum of 48 characters.
</code> | ||
<code title="Invalid: Using PHP's header() function for redirection."> | ||
<![CDATA[ | ||
header( "Location: $url" ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use <em>
tags to highlight the "good" and the "bad" bits in the code examples. More information in the description of #1722.
</code> | ||
<code title="Invalid: Using PHP's header() function for redirection."> | ||
<![CDATA[ | ||
header( "Location: $url" ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct me if I'm missing something, but as far as I can test, the use of header()
does not trigger the WordPress.WP.AlternativeFunctions
. I created a file with the contents of this invalid example and run PHPCS but got no errors:
$ cat test.inc
<?php
header( "Location: $url" );
$ vendor/bin/phpcs --standard=WordPress test.inc -s --sniffs=WordPress.WP.AlternativeFunctions
$
I haven't tested all the other <code_comparison>
blocks, but on a quick look, it seems to me file_get_contents()
does this this sniff, but the others don't. Now I'm starting to question my self if I'm checking the correct sniff. This is the one that I'm looking:
final class AlternativeFunctionsSniff extends AbstractFunctionRestrictionsSniff { |
Related to: #1722