Skip to content
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

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

pamprn09
Copy link

@pamprn09 pamprn09 commented Sep 18, 2024

Related to: #1722

Copy link
Collaborator

@rodrigoprimo rodrigoprimo left a 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"?>
Copy link
Collaborator

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

Comment on lines +47 to +50
<![CDATA[
_e( 'Hello, World!', 'text-domain' );
$greeting = __( 'Hello, World!', 'text-domain' );
]]>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<![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.

Screenshot from 2024-09-26 11-50-07

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" );
Copy link
Collaborator

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" );
Copy link
Collaborator

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 {
. Before I continue this review, could you please check and get back to me?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants