Make WordPress Core

Opened 6 months ago

Last modified 6 months ago

#63621 new enhancement

Enhancement: Ensure Hello Dolly passess the Plugin Check (PCP)

Reported by: jhimross's profile jhimross Owned by:
Milestone: Awaiting Review Priority: normal
Severity: minor Version:
Component: Plugins Keywords: has-patch has-test-info has-screenshots
Focuses: coding-standards Cc:

Description

This ticket details several issues found in the Hello Dolly plugin, along with proposed solutions to ensure it passes the Plugin Check (PCP).

See screenshot when I tested it using Plugin Check. https://p-d0fk22zg.t2.n0.cdn.zight.com/items/rRuK26b5/31fc4cc1-3155-45de-ac75-9fa87ab08024.jpeg
And here is the screenshot after I applied the changes below: https://p-d0fk22zg.t2.n0.cdn.zight.com/items/12umW1ox/bd3ceef7-17f2-41f8-b0e8-9a9a2a0066aa.jpeg

Issue 1: Missing Text Domain in () Function

Problem: The () function on line 67 is missing the required $domain parameter for internationalization, which is a PCP requirement.

Fix: Add 'hello-dolly' as the text domain.

// Original:
__( 'Quote from Hello Dolly song, by Jerry Herman:' ),
// Fixed:
__( 'Quote from Hello Dolly song, by Jerry Herman:', 'hello-dolly' ),

Issue 2: Missing License Information in Plugin Header

Problem: The plugin header is missing "License" and "License URI" information, which is a standard PCP check.

Fix: Add the following lines to the plugin header, ideally before Author: Matt Mullenweg.

/*
Plugin Name: Hello Dolly
Plugin URI: http://wordpress.org/plugins/hello-dolly/
Description: This is not just a plugin, it symbolizes the hope and enthusiasm of an entire generation summed up in two words sung most famously by Louis Armstrong: Hello, Dolly. When activated you will randomly see a lyric from <cite>Hello, Dolly</cite> in the upper right of your admin screen on every page.
Author: Matt Mullenweg
Version: 1.7.2
License: GPLv2 or later
License URI: https://www.gnu.org/licenses/gpl-2.0.html
Author URI: http://ma.tt/
*/

Issue 3: Unescaped Output

Problem: On lines 67, 68, and 69, output is not being properly escaped, leading to potential Cross-Site Scripting (XSS) vulnerabilities. This is a critical security check in PCP.

Fix: Apply appropriate escaping functions:

For the translatable string on line 67, use esc_html().

For $lang on line 68 (used within an HTML attribute), use esc_attr().

For $chosen on line 69 (plain text output), use esc_html().

// Original:
printf(
    '<p id="dolly"><span class="screen-reader-text">%s </span><span dir="ltr"%s>%s</span></p>',
    __( 'Quote from Hello Dolly song, by Jerry Herman:' ), // Line 67
    $lang, // Line 68
    $chosen // Line 69
);
// Fixed:
printf(
    '<p id="dolly"><span class="screen-reader-text">%s </span><span dir="ltr"%s>%s</span></p>',
    esc_html__( 'Quote from Hello Dolly song, by Jerry Herman:', 'hello-dolly' ), // Line 67
    esc_attr( $lang ), // Line 68
    esc_html( $chosen ) // Line 69
);

Issue 4: Discouraged mt_rand() Function

Problem: On line 54, mt_rand() is used, which is discouraged in WordPress for consistency and better randomness, as highlighted by PCP.

Fix: Replace mt_rand() with wp_rand().

// Original:
return wptexturize( $lyrics[ mt_rand( 0, count( $lyrics ) - 1 ) ] );
// Fixed:
return wptexturize( $lyrics[ wp_rand( 0, count( $lyrics ) - 1 ) ] );

Attachments (1)

hello.zip (1.5 KB) - added by jhimross 6 months ago.
updated hello.php with the patch

Download all attachments as: .zip

Change History (4)

@jhimross
6 months ago

updated hello.php with the patch

This ticket was mentioned in PR #9081 on WordPress/wordpress-develop by @jhimross.


6 months ago
#1

This PR addresses several identified issues in the hello.php file of the Hello Dolly plugin to improve code quality, security, and ensure that it passes the Plugin Check (PCP).

Key Changes:

1.Internationalization (i18n) Improvements:
Added the correct text domain (hello-dolly) to the () function call on line 67, resolving the Missing $domain parameter error.

Note on TextDomainMismatch warning: While a linter might report TextDomainMismatch on line 69 expecting plugins, hello-dolly is the correct and specific text domain for this plugin. The linter warning has been suppressed using phpcs:ignore WordPress.WP.I18n.TextDomainMismatch.

2.Plugin Header Compliance:
Added License: GPLv2 or later and License URI: https://www.gnu.org/licenses/gpl-2.0.html to the plugin header, resolving the Missing "License" in Plugin Header error.

3.Output Escaping for Security:
Implemented proper escaping for all output printed to the screen on lines 67, 68, and 69 to prevent Cross-Site Scripting (XSS) vulnerabilities.

Used esc_html() for translatable HTML content.

Used esc_attr() for HTML attribute values.

Used esc_html() for general text output.

4.Random Number Generation Best Practice:
Replaced mt_rand() with wp_rand() on line 54 for improved randomness and adherence to WordPress's recommended functions.

Trac ticket:

---
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

WordPress Trac Ticket: https://core.trac.wordpress.org/ticket/63621

This ticket was mentioned in Slack in #core-test by oglekler. View the logs.


6 months ago

#3 @oglekler
6 months ago

  • Focuses coding-standards added

This ticket was discussed during the Test team triage session.

We have several tickets about Hello Dolly, and from my point of view they have valid points and can be incorporated into one patch, so I am linking them for further discussion.
#61215, #61069, #58363, #53323

Last edited 6 months ago by oglekler (previous) (diff)
Note: See TracTickets for help on using tickets.