WordPress.org

Make WordPress Core

Opened 19 months ago

Last modified 18 months ago

#43764 new defect (bug)

CS: Fix violations for admin-header.php

Reported by: marcomartins Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: General Keywords:
Focuses: coding-standards Cc:
PR Number:

Description

Fixes the coding standard violations for wp-admin/admin-header.php

Ignores the following sniffs:

  • Generic.PHP.NoSilencedErrors.Discouraged
  • WordPress.NamingConventions.ValidHookName.UseUnderscores
  • WordPress.Variables.GlobalVariables.OverrideProhibited

Attachments (3)

43764.patch (2.4 KB) - added by marcomartins 19 months ago.
43764.2.patch (2.8 KB) - added by marcomartins 19 months ago.
Inline phpcs:ignore moved to its own line or replaced with phpcs:disable.
43764.3.patch (2.7 KB) - added by marcomartins 19 months ago.
Fixes WordPress.WhiteSpace.PrecisionAlignment.Found and a faulty phpcs:ignore

Download all attachments as: .zip

Change History (5)

@marcomartins
19 months ago

Inline phpcs:ignore moved to its own line or replaced with phpcs:disable.

@marcomartins
19 months ago

Fixes WordPress.WhiteSpace.PrecisionAlignment.Found and a faulty phpcs:ignore

#1 @jrf
19 months ago

@marcomartins Welcome as a new contributor and thank you for your efforts.

I've reviewed the latest patch (3).

  1. Based on your patch I realize there is actually an issue with the ruleset as currently committed to core. The WordPress.Variables.GlobalVariables sniff is referenced there, but should not be as that sniff should not be part of the core ruleset. Core overwriting Core global variables should always be fine. So this should be fixed in the ruleset, not by using phpcs:ignore comments.
  2. The hook names cannot be ignored for "allow for dash in dynamic hook". The handbook is quite clear that dashes are not allowed in hook names: https://make.wordpress.org/core/handbook/best-practices/coding-standards/php/#naming-conventions, including dynamic hook names (see the examples) https://make.wordpress.org/core/handbook/best-practices/coding-standards/php/#interpolation-for-naming-dynamic-hooks

So there are two options here:

  • Either ignore it for the right reasons, i.e. not to break backwards-compatibility.
  • Or deprecated the existing hook and introduce a new hook which complies with the naming conventions.
  1. The disable comments can be combined within the hook documentation blocks, like so:
    <?php
    /**
     * Fires when scripts are printed for a specific admin page based on $hook_suffix.
     *
     * @since 2.1.0
     *
     * @phpcs:disable WordPress.NamingConventions.ValidHookName.UseUnderscores -- left as is not to break BC.
     */
    do_action( "admin_print_scripts-{$hook_suffix}" );
    //phpcs:enable
    

which is a "cleaner" solution than having a // comment above the docblock.

#2 @jrf
18 months ago

FYI: point 1 of my remarks is being addressed in ticket #43776

Note: See TracTickets for help on using tickets.