[wp-trac] [WordPress Trac] #43764: CS: Fix violations for admin-header.php

[WordPress Trac]

#43764: CS: Fix violations for admin-header.php
--------------------------+-------------------------------
 Reporter:  marcomartins  |       Owner:  (none)
     Type:  defect (bug)  |      Status:  new
 Priority:  normal        |   Milestone:  Awaiting Review
Component:  General       |     Version:
 Severity:  normal        |  Resolution:
 Keywords:                |     Focuses:  coding-standards
--------------------------+-------------------------------

Comment (by jrf):

 @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.

 3. The `disable` comments can be combined within the hook documentation
 blocks, like so:
 {{{#!php
 <?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.

-- 
Ticket URL: <https://core.trac.wordpress.org/ticket/43764#comment:1>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform