WordPress.org

Make WordPress Core

Opened 22 months ago

Closed 22 months ago

Last modified 14 months ago

#43150 closed defect (bug) (fixed)

Update Emoji build step to follow WP Coding Standards.

Reported by: peterwilsoncc Owned by: pento
Milestone: 5.1 Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords: has-patch
Focuses: coding-standards Cc:
PR Number:

Description

The Emoji build step modifying src/wp-includes/formatting.php requires an update to follow the WordPress PHP coding standards. Alternatively the built portion of the file can be ignored.

Attachments (3)

43150.diff (1.2 KB) - added by netweb 22 months ago.
43150.2.diff (74.8 KB) - added by netweb 22 months ago.
43150.3.diff (1.2 KB) - added by netweb 22 months ago.

Download all attachments as: .zip

Change History (14)

#1 @pento
22 months ago

Ignore the built portion.

@netweb
22 months ago

#2 @netweb
22 months ago

  • Keywords has-patch commit added; needs-patch removed

Patch 43150.diff updates the formmatting within the Grunt replace emoji task .

This change updates the Grunt task to print the array of Twemoji imported to match that of WordPress' PHP Coding Standards.

Here's a truncated view of _wp_emoji_list() after running grunt replace using the 43150.diff patch:

<?php
function _wp_emoji_list( $type = 'entities' ) {
        // Do not remove the START/END comments - they're used to find where to insert the arrays.

        // START: emoji arrays
        $entities = array(
                '&#x1f469;&#x200d;&#x2764;&#xfe0f;&#x200d;&#x1f48b;&#x200d;&#x1f469;',
                '&#x1f469;&#x200d;&#x2764;&#xfe0f;&#x200d;&#x1f48b;&#x200d;&#x1f468;',
                '&#x1f468;&#x200d;&#x2764;&#xfe0f;&#x200d;&#x1f48b;&#x200d;&#x1f468;',
                ...
                ...
                '&#x2049;',
                '&#x203c;',
                '&#xe50a;',
        );
        $partials = array(
                '&#x1f004;',
                '&#x1f0cf;',
                '&#x1f170;',
                ...
                ...
                '&#x3297;',
                '&#x3299;',
                '&#xe50a;',
        );
        // END: emoji arrays

PHPCS Stats via: phpcs --standard=phpcs.xml.dist --report-summary --report-source src/wp-includes/formatting.php -v

Before:

----------------------------------------------------------------------------
    SOURCE                                                             COUNT
----------------------------------------------------------------------------
[x] WordPress.Arrays.CommaAfterArrayItem.NoSpaceAfterComma             3863
[ ] WordPress.PHP.YodaConditions.NotYoda                               19
[ ] Squiz.PHP.DisallowMultipleAssignments.Found                        4
[ ] Generic.PHP.NoSilencedErrors.Discouraged                           3
[x] WordPress.Arrays.ArrayDeclarationSpacing.NoSpaceBeforeArrayCloser  2
[x] WordPress.Arrays.ArrayDeclarationSpacing.NoSpaceAfterArrayOpener   2
[ ] WordPress.Variables.GlobalVariables.OverrideProhibited             2
[ ] WordPress.NamingConventions.ValidVariableName.NotSnakeCase         2
[ ] WordPress.WhiteSpace.PrecisionAlignment.Found                      2
[ ] WordPress.NamingConventions.ValidFunctionName.FunctionNameInvalid  1
----------------------------------------------------------------------------
A TOTAL OF 3900 SNIFF VIOLATIONS WERE FOUND IN 10 SOURCES

After:

PHP CODE SNIFFER VIOLATION SOURCE SUMMARY
----------------------------------------------------------------------------
SOURCE                                                                 COUNT
----------------------------------------------------------------------------
WordPress.PHP.YodaConditions.NotYoda                                   19
Squiz.PHP.DisallowMultipleAssignments.Found                            4
Generic.PHP.NoSilencedErrors.Discouraged                               3
WordPress.Variables.GlobalVariables.OverrideProhibited                 2
WordPress.NamingConventions.ValidVariableName.NotSnakeCase             2
WordPress.WhiteSpace.PrecisionAlignment.Found                          2
WordPress.NamingConventions.ValidFunctionName.FunctionNameInvalid      1
----------------------------------------------------------------------------
A TOTAL OF 33 SNIFF VIOLATIONS WERE FOUND IN 7 SOURCES
----------------------------------------------------------------------------

Edit: After applying the patch grunt replace needs to be run to update src/wp-includes/formatting.php

Last edited 22 months ago by netweb (previous) (diff)

#3 @GaryJ
22 months ago

+1 for the simple fix to address 3863 CS violations in the generated code. Just because it happens to be written programmatically rather than by hand, doesn't mean it shouldn't follow WPCS.

#4 @pento
22 months ago

  • Keywords commit removed

We're definitely not adding 4000 lines of array elements to formatting.php, just to satisfy WPCS.

That's why I said to ignore the built lines, because that's the only practical option.

@netweb
22 months ago

#5 @netweb
22 months ago

43150.2.diff adds the phpcs:disable to avoid any PHPCS warnings for the built lines to src/wp-includes/formatting.php and updates the format to be:

<?php
function _wp_emoji_list( $type = 'entities' ) {
        // Do not remove the START/END comments - they're used to find where to insert the arrays.

        // phpcs:disable WordPress.Arrays.ArrayDeclarationSpacing.ArrayItemNoNewLine -- don't format the array across 4000 lines
        // phpcs:disable WordPress.Arrays.CommaAfterArrayItem.NoSpaceAfterComma

        // START: emoji arrays
        $entities = array(
                '&#x1f469;&#x200d;&#x2764;&#xfe0f;&#x200d;&#x1f48b;&#x200d;&#x1f469;','&#x1f469;&#x200d;&#x2764;&#xfe0f;&#x200d;&#x1f48b;&#x200d;&#x1f468;',...
        );
        $partials = array(
                '&#x1f004;','&#x1f0cf;','&#x1f170;','&#x1f171;','&#x1f17e;','&#x1f17f;','&#x1f18e;','&#x1f191;','&#x1f192;','&#x1f193;','&#x1f194;','&#x1f195;',...
        );
        // END: emoji arrays

        // phpcs:enable

#6 follow-up: @jrf
22 months ago

@netweb FYI, you can combine several disables in one command, so the comment block above the arrays could look like this:

<?php
        /*
         * Don't format the array across 4000 lines.
         * phpcs:disable WordPress.Arrays.ArrayDeclarationSpacing.ArrayItemNoNewLine,WordPress.Arrays.CommaAfterArrayItem.NoSpaceAfterComma
         * 
         * N.B.: Do not remove the START/END comments - they're used to find where to insert the arrays.
         */

As a side-note: as this is a non-keyed array, as far as I can see, WPCS wouldn't be throwing the WordPress.Arrays.ArrayDeclarationSpacing.ArrayItemNoNewLine error if the array was passed like array(....) (no new line after opening bracket or before closing bracket).
The line is going to be ridiculously long anyway, so why have those two new lines ?

Similarly, as the line is so long, why not have the comma's after each array item ?

Having said that, you may not need to use the phpcs:disable comments at all to still comply with the standards.

@netweb
22 months ago

#7 in reply to: ↑ 6 @netweb
22 months ago

Replying to jrf:

@netweb FYI, you can combine several disables in one command, so the comment block above the arrays could look like this:

<?php
        /*
         * Don't format the array across 4000 lines.
         * phpcs:disable WordPress.Arrays.ArrayDeclarationSpacing.ArrayItemNoNewLine,WordPress.Arrays.CommaAfterArrayItem.NoSpaceAfterComma
         * 
         * N.B.: Do not remove the START/END comments - they're used to find where to insert the arrays.
         */

Neat, thanks @jrf, I went searching the PHPCS docs to find some documentation or examples of multiline phpcs:disable and didn't find any, this is much neater and will keep this in mind for the future +1

As a side-note: as this is a non-keyed array, as far as I can see, WPCS wouldn't be throwing the WordPress.Arrays.ArrayDeclarationSpacing.ArrayItemNoNewLine error if the array was passed like array(....) (no new line after opening bracket or before closing bracket).
The line is going to be ridiculously long anyway, so why have those two new lines ?

Similarly, as the line is so long, why not have the comma's after each array item ?

Having said that, you may not need to use the phpcs:disable comments at all to still comply with the standards.

Patch 43150.3.diff implements @jrf's approach above, no phpcs:disable comments are required in src/wp-includes/formatting.php and is now formatted as:

<?php
function _wp_emoji_list( $type = 'entities' ) {
        // Do not remove the START/END comments - they're used to find where to insert the arrays.

        // START: emoji arrays
        $entities = array( '&#x1f469;&#x200d;&#x2764;&#xfe0f;&#x200d;&#x1f48b;&#x200d;&#x1f469;', '&#x1f469;&#x200d;&#x2764;&#xfe0f;&#x200d;&#x1f48b;&#x200d;&#x1f468;', '&#x1f468;&#x20...
        $partials = array( '&#x1f004;', '&#x1f0cf;', '&#x1f170;', '&#x1f171;', '&#x1f17e;', '&#x1f17f;', '&#x1f18e;', '&#x1f191;', '&#x1f192;', '&#x1f193;', '&#x1f194;', '&#x1f195;', '...
        // END: emoji arrays

#8 @jrf
22 months ago

👍

@netweb You can find the documentation about the PHPCS annotations here:
https://github.com/squizlabs/PHP_CodeSniffer/wiki/Advanced-Usage#ignoring-parts-of-a-file

And here's the initial release info which also mentions it: https://github.com/squizlabs/PHP_CodeSniffer/releases/tag/3.2.0

#9 @pento
22 months ago

  • Owner set to pento
  • Status changed from new to assigned

#10 @pento
22 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 42610:

Emoji: Tweak the generated emoji arrays to not cause coding standards errors.

Props netweb, jrf.
Fixes #43150.

#11 @jorbin
14 months ago

  • Milestone changed from 5.0 to 5.1
Note: See TracTickets for help on using tickets.