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.

Sign up for GitHub

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

Adding documentation to WordPress.WP.DiscouragedConstants #2493

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

Adding documentation to WordPress.WP.DiscouragedConstants #2493

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 49 additions & 0 deletions WordPress/Docs/WP/DiscouragedConstantsStandard.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
<?xml version="1.0"?>
<documentation xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:noNamespaceSchemaLocation="https://phpcsstandards.github.io/PHPCSDevTools/phpcsdocs.xsd"
title="Discouraged Constants"
>
<standard>
<![CDATA[
The usage of WordPress Constants is discouraged.
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
The usage of WordPress Constants is discouraged.
The usage of WordPress constants is discouraged.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if it makes sense to list here and in the other <standard> block the discouraged constants, as this sniff triggers a warning only for a specific set of WP constants and not all constants. What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

On a second thought, maybe providing the whole list is too much, we could simply say something like The use of certain WordPress constants is discouraged.

]]>
</standard>
<code_comparison>
<code title="Valid: A function is used in the code to retrieve the theme directory.">
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not explicitly mentioned in #1722, but my understanding, based on checking the documentation from other sniffs, is that the description of the valid and invalid examples should be generic and describe what constitutes a valid example for this particular check that the sniff performs instead of describing the code sample provided.

Maybe here, the valid description should mention that the code does not use one of the discouraged WP constants or something like that?

<![CDATA[
<img src="
Copy link
Collaborator

Choose a reason for hiding this comment

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

AFAIK, it is not common to indent an HTML tag like it is done here. Maybe instead of fixing the indentation, we can simplify the example and make it easier to understand what this sniff is about? I believe the code sample could be trimmed down to echo <em>TEMPLATEPATH</em>;.

Copy link
Collaborator

@rodrigoprimo rodrigoprimo Oct 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Actually just echo <em>TEMPLATEPATH</em>; is not a realistic example. Maybe something like the example below is better?

$subdir = TEMPLATEPATH . '/subdir/';

And something similar for the example with get_template_directory_uri().

<?php echo <em>get_template_directory_uri()</em>; ?>
/img/logo.svg" />
]]>
</code>
<code title="Invalid: The TEMPLATEPATH constant is used in the code.">
<![CDATA[
<img src="
<?php echo <em>TEMPLATEPATH</em>;?>
/img/logo.svg" />
]]>
</code>
</code_comparison>
<standard>
<![CDATA[
Defining WordPress Constants is discouraged.
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
Defining WordPress Constants is discouraged.
Defining WordPress constants is discouraged.

]]>
</standard>
<code_comparison>
<code title="Valid: A function is used in the code to retrieve the theme directory.">
<![CDATA[
<img src="
<?php echo <em>get_template_directory_uri()</em>; ?>
/img/logo.svg" />
]]>
</code>
<code title="Invalid: The TEMPLATEPATH constant is set by a theme.">
<![CDATA[
<em>define("TEMPLATEPATH", "foo");</em>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe it is better to highlight just the constant name instead of the whole line as it is not any call to define() that triggers this sniff?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would suggest using a different discouraged constant here, so that we have two examples of constants that trigger this sniff. And possibly wrapping it with a if defined check to illustrate that the sniff is triggered even in this case.

<img src="
<?php echo <em>$foo</em>; ?>
/img/logo.svg" />
Comment on lines +42 to +45
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think it is necessary to include the <img> tag here. Including it means that this code example will trigger both sniff warnings and not just the one associated with this <standard> block. If you decide to keep the <img>, there are some fixes that need to be made. This code currently generates a syntax error due to the missing PHP closing tag after the call to define(). Also, I believe it should echo TEMPLATEPATH instead of the undefined variable $foo.

]]>
</code>
</code_comparison>
</documentation>
Loading