Skip to content

Fix the code examples #92

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

Merged
merged 6 commits into from
Jul 17, 2022
Merged

Fix the code examples #92

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
69a81e6
Fix the code examples
dingo-d Jun 17, 2022
246b71a
Update wordpress-coding-standards/php.md
dingo-d Jun 17, 2022
01a7e76
Update wordpress-coding-standards/php.md
dingo-d Jun 17, 2022
1b6b4aa
Update wordpress-coding-standards/php.md
dingo-d Jun 17, 2022
0c2563a
Update wordpress-coding-standards/php.md
dingo-d Jun 17, 2022
ea85a03
Update code example to include the text domain
dingo-d Jul 3, 2022
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
87 changes: 45 additions & 42 deletions wordpress-coding-standards/php.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@ If you want to automatically check your code against this standard, you can use
Use single and double quotes when appropriate. If you're not evaluating anything in the string, use single quotes. You should almost never have to escape quotes in a string, because you can just alternate your quoting style, like so:

```php
echo '<a href="/static/link" title="Yeah yeah!">Link name</a>';
echo "<a href='$link' title='$linktitle'>$linkname</a>";
echo '<a href="/static/link" class="button button-primary">Link name</a>';
echo "<a href='$escaped_link'>text with a ' single quote</a>";
```

Text that goes into HTML or XML attributes should be escaped so that single or double quotes do not end the attribute value and invalidate the HTML, causing a security issue. See [Data Validation](https://developer.wordpress.org/plugins/security/data-validation/) in the Plugin Handbook for further details.
Expand Down Expand Up @@ -119,7 +119,7 @@ Note that requiring the use of braces means that _single-statement inline contro
<?php if ( have_posts() ) : ?>
<div class="hfeed">
<?php while ( have_posts() ) : the_post(); ?>
<article id="post-<?php the_ID() ?>" class="<?php post_class() ?>">
<article id="<?php echo esc_attr( 'post-' . get_the_ID() ); ?>" class="<?php echo esc_attr( get_post_class() ); ?>">
<!-- ... -->
</article>
<?php endwhile; ?>
Expand Down Expand Up @@ -166,7 +166,7 @@ $bar = array(
);
$baz = sprintf(
/* translators: %s: Friend's name */
esc_html__( 'Hello, %s!', 'yourtextdomain' ),
__( 'Hello, %s!', 'yourtextdomain' ),
$friend_name
);

Expand All @@ -192,16 +192,18 @@ Correct (Multiline):

```php
function foo() {
?>
<div>
<?php
echo bar(
$baz,
$bat
);
?>
</div>
<?php
?>
<div>
<?php
echo esc_html(
bar(
$baz,
$bat
)
);
?>
</div>
<?php
Copy link
Member

Choose a reason for hiding this comment

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

This may need a check that the tabs in code examples in markdown are handled correctly when the page gets converted to the handbook.

In my experience spaces are the way to go with markdown, but this is kind of a special set up, so I'm not sure.

}
```

Expand All @@ -227,14 +229,14 @@ Correct:

```php
<?php ... ?>
<?php echo $var; ?>
<?php echo esc_html( $var ); ?>
```

Incorrect:

```php
<? ... ?>
<?= $var ?>
<?= esc_html( $var ) ?>
```

## Remove Trailing Spaces
Expand All @@ -246,12 +248,12 @@ Remove trailing whitespace at the end of each line. Omitting the closing PHP tag
Always put spaces after commas, and on both sides of logical, comparison, string and assignment operators.

```php
x === 23
foo && bar
! foo
array( 1, 2, 3 )
$baz . '-5'
$term .= 'X'
SOME_CONST === 23;
foo() && bar();
! $foo;
array( 1, 2, 3 );
$baz . '-5';
$term .= 'X';
```

Put spaces on both sides of the opening and closing parentheses of control structure blocks.
Expand Down Expand Up @@ -292,22 +294,22 @@ $foo = (bool) $bar;
When referring to array items, only include a space around the index if it is a variable, for example:

```php
$x = $foo['bar']; // correct
$x = $foo[ 'bar' ]; // incorrect
$x = $foo['bar']; // Correct.
$x = $foo[ 'bar' ]; // Incorrect.

$x = $foo[0]; // correct
$x = $foo[ 0 ]; // incorrect
$x = $foo[0]; // Correct.
$x = $foo[ 0 ]; // Incorrect.

$x = $foo[ $bar ]; // correct
$x = $foo[$bar]; // incorrect
$x = $foo[ $bar ]; // Correct.
$x = $foo[$bar]; // Incorrect.
```

In a `switch` block, there must be no space between the `case` condition and the colon.

```php
switch ( $foo ) {
case 'bar': // correct
case 'ba' : // incorrect
case 'bar': // Correct.
case 'ba' : // Incorrect.
}
```

Expand Down Expand Up @@ -336,8 +338,8 @@ Functions that update the database should expect their parameters to lack SQL sl
`$wpdb->prepare()` is a method that handles escaping, quoting, and int-casting for SQL queries. It uses a subset of the `sprintf()` style of formatting. Example :

```php
$var = "dangerous'"; // raw data that may or may not need to be escaped
$id = some_foo_number(); // data we expect to be an integer, but we're not certain
$var = "dangerous'"; // Raw data that may or may not need to be escaped.
$id = some_foo_number(); // Data we expect to be an integer, but we're not certain.

$wpdb->query( $wpdb->prepare( "UPDATE $wpdb->posts SET post_title = %s WHERE ID = %d", $var, $id ) );
```
Expand Down Expand Up @@ -421,20 +423,20 @@ class Example_Class_Extended { [...] }
Prefer string values to just `true` and `false` when calling functions.

```php
// Incorrect
// Incorrect.
function eat( $what, $slowly = true ) {
...
}
eat( 'mushrooms' );
eat( 'mushrooms', true ); // what does true mean?
eat( 'dogfood', false ); // what does false mean? The opposite of true?
eat( 'mushrooms', true ); // What does true mean?
eat( 'dogfood', false ); // What does false mean? The opposite of true?
```

PHP only supports named arguments as of PHP 8.0. However, as WordPress currently still supports older PHP versions, we cannot yet use those.
Without named arguments, the values of the flags are meaningless, and each time we come across a function call like the examples above, we have to search for the function definition. The code can be made more readable by using descriptive string values, instead of booleans.

```php
// Correct
// Correct.
function eat( $what, $speed = 'slowly' ) {
...
}
Expand All @@ -446,13 +448,14 @@ eat( 'dogfood', 'quickly' );
When more words are needed to describe the function parameters, an `$args` array may be a better pattern.

```php
// Even Better
function eat( $what, $args ) {
...
}
eat ( 'noodles', array( 'speed' => 'moderate' ) );
```

Be careful when using this pattern, as it can lead to "Undefined array index" notices in your code if proper checks are not set in place. Use this pattern only where it makes sense (i.e. multiple possible arguments), not just for the sake of it.

## Interpolation for Naming Dynamic Hooks

Dynamic hooks should be named using interpolation rather than concatenation for readability and discoverability purposes.
Expand Down Expand Up @@ -519,15 +522,15 @@ Correct:

```php
if ( 0 === strpos( 'WordPress', 'foo' ) ) {
echo __( 'Yay WordPress!' );
echo esc_html__( 'Yay WordPress!' );
Copy link
Member

Choose a reason for hiding this comment

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

Should one of these examples have a textdomain?

Copy link
Member Author

Choose a reason for hiding this comment

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

If we're looking at it as a WordPress core code, then no. If we are looking at it as an included theme, then yeah, we should add the textdomain.

Copy link
Member

Choose a reason for hiding this comment

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

Good point. Not sure what the answer should be as what's described in the handbook are the coding standards for Core primarily (and recommended for the community secondarily).

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd leave it without because it's mostly intended to be used for WP core.

Do you agree with that @GaryJones ?

Copy link
Member

Choose a reason for hiding this comment

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

The purpose of the WordPress Coding Standards is to create a baseline for collaboration and review within various aspects of the WordPress open source project and community, from core code to themes to plugins.

The WordPress community developed the standards contained in this section of the handbook, and those standards are part of the best practices that developers and core contributors are recommended to follow.
— https://developer.wordpress.org/coding-standards/wordpress-coding-standards/

I bet there would be far more non-Core developers (i.e. plugin and theme developers) looking at these docs, than WP Core developers or Core contributors, so if we're going to emphasise best practices and expectations, and we have examples like this which already includes textdomains, then it would make sense to be consistent?

Copy link
Member

Choose a reason for hiding this comment

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

We're also using esc_html__() here, yet Core would use __() as they don't consider bad/suspicious translations to be a risk, so it's clearly tailored towards non-Core developers; not including the textdomain makes it an example that isn't accurate for either group of developers.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can add a textdomain, not a big problem 🙂

Would this be better:

	echo esc_html__( 'Yay WordPress!', 'textdomain' );

Or should we do something more specific for the textdomain name?

}
```

Incorrect:

```php
if ( 0 == strpos( 'WordPress', 'foo' ) ) {
echo __( 'Yay WordPress!' );
echo esc_html__( 'Yay WordPress!' );
}
```

Expand All @@ -538,15 +541,15 @@ Correct:
```php
$data = $wpdb->get_var( '...' );
if ( $data ) {
// Use $data
// Use $data.
}
```

Incorrect:

```php
if ( $data = $wpdb->get_var( '...' ) ) {
// Use $data
// Use $data.
}
```

Expand All @@ -556,7 +559,7 @@ In a `switch` statement, it's okay to have multiple empty cases fall through to
switch ( $foo ) {
case 'bar': // Correct, an empty case can fall through without comment.
case 'baz':
echo $foo; // Incorrect, a case with a block must break, return, or have a comment.
echo esc_html( $foo ); // Incorrect, a case with a block must break, return, or have a comment.
case 'cat':
echo 'mouse';
break; // Correct, a case with a break does not require a comment.
Expand Down