Update WPCS to 1.0.0, and enforce coding standards

[pento]

WPCS 1.0.0 will be released very soon, which is a lovely round version number to update to.

This is a tracking ticket to ensure the necessary tasks are taken care of.

• Apply any necessary manual fixes prior to the next formatting run.
• Update composer.json to the new version, and phpcs.xml.dist with any tweaks.
• Apply the formatting run.
• Create a Grunt precommit task to run composer format on any changed PHP files.
• Create a Travis job to ensure auto-fixable code formatting errors cannot be re-introduced.

[pento]

For the phpcs.xml.dist changes, I've proposed a change in #43644, requiring each parameter of a multi-line function call to be on a new line.

We also need to add a rule to protect the special line endings in the config files:

	<rule ref="Generic.Files.LineEndings">
		<exclude-pattern>/wp-config\.php</exclude-pattern>
		<exclude-pattern>/wp-config-sample\.php</exclude-pattern>
	</rule>

[pento]

Gruntfile precommit format job

[pento]

44600.diff​ is a Gruntfile job that runs composer format on changed files when grunt precommit is run. Changes won't cause the job to fail.

When grunt prerelease is run, it will run across all files, and changes are considered to be a job fail.

[azaozz]

Replying to pento:

For the phpcs.xml.dist changes, I've proposed a change in #43644, requiring each parameter of a multi-line function call to be on a new line.

I'm actually thinking we should *ban* multi-line function parameters. There is no good reason to have them, they just make the code harder to read.

Will comment on #43644.

[jrf]

FYI: WPCS 1.0.0 has been released in the mean time.
Changelog: ​https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/releases/tag/1.0.0

Regarding the sniff name changes in WPCS 1.0.0, no changes are needed in the current custom ruleset use by WP Core.

[pento]

44600.2.diff​ is the changes to composer.json and phpcs.xml.dist.

[pento]

In 43569:

Coding Standards: Prepare for upgrading WPCS to 1.0.0.

In order to get the best result when running phpcbf across the codebase, there are some manual tweaks we need to make.

These fall into three categories:

• Fixing incorrectly indented code which has flow-on effects when auto-fixing.
• Tweaking the layout of inline PHP inside HTML tags.
• Moving more complex inline PHP inside HTML tags, to execute earlier.

See #44600.

[jrf]

@pento

Re: patch 44600.2.diff

• I can understand fixing WPCS to a certain version, though as WPCS will be using semantic versioning, I think ~1.0.0 can safely be used instead of 1.0.0 as - if there will be a 1.0.x release, it will only contain bug fixes. New sniffs will not be introduced in patch releases, only in minors/majors.
• I don't understand why the DealerDirect Composer should be fixed to a certain version. This is a plugin to handle setting the PHPCS installed_paths, so it should be fine to always use the latest version available.
• PEAR.Functions.FunctionCallSignature config: The requiredSpaces properties are already set to 1 in the WordPress-Core WPCS ruleset, so you don't need to set them in the custom ruleset.
• As for the allowMultipleArguments property for that same sniff, I have two questions:
  • Should that configuration be set in the custom ruleset or in the WordPress-Core ruleset ?
  • And is that configuration only needed for correctly *fixing* things or should the related error also be thrown when running phpcs ?

Re: fixing the non-autofixable issues:

• ​WPCS PR 1450 takes care of 119 issues which could be regarded as false positives.
• ​PHPCS PR 2121 which has been merged and will be included in the PHPCS 3.3.2 release, takes care of another 24 false positives.

[pento]

Replying to jrf:

I can understand fixing WPCS to a certain version, though as WPCS will be using semantic versioning, I think ~1.0.0 can safely be used instead of 1.0.0 as - if there will be a 1.0.x release, it will only contain bug fixes. New sniffs will not be introduced in patch releases, only in minors/majors.

Solely for reproducibility. I'm not concerned about new sniffs, but if a sniff bugfix changed the behaviour, it would cause problems for anything relying on that behaviour to not change.

I don't understand why the DealerDirect Composer should be fixed to a certain version. This is a plugin to handle setting the PHPCS installed_paths, so it should be fine to always use the latest version available.

For consistency. Whilst building Gutenberg, we've found it to be far safer to use exact version numbers for all packages, as behaviour can unexpectedly change, even in bugfix releases. Rather than choose packages that can be safely allowed to use the latest version, it's better to fix all version numbers, and upgrade as we need.

PEAR.Functions.FunctionCallSignature config: The requiredSpaces properties are already set to 1 in the WordPress-Core WPCS ruleset, so you don't need to set them in the custom ruleset.

Huh, I thought it broke something when I tried to do that, but it works fine. Guess I must've messed something else up in earlier testing. 🙂

• As for the allowMultipleArguments property for that same sniff, I have two questions:
  • Should that configuration be set in the custom ruleset or in the WordPress-Core ruleset ?

Yah, it can be moved to WordPress-Core. Can WPCS releases move a bit faster than they have previously? Waiting weeks for a new release to make a config change is not ideal.

• And is that configuration only needed for correctly *fixing* things or should the related error also be thrown when running phpcs ?

Sure, it should probably throw there, too.

[jrf]

Solely for reproducibility. I'm not concerned about new sniffs, but if a sniff bugfix changed the behaviour, it would cause problems for anything relying on that behaviour to not change.

For consistency. Whilst building Gutenberg, we've found it to be far safer to use exact version numbers for all packages, as behaviour can unexpectedly change, even in bugfix releases. Rather than choose packages that can be safely allowed to use the latest version, it's better to fix all version numbers, and upgrade as we need.

That's what the composer.lock file is for.... or am I missing something ?

Can WPCS releases move a bit faster than they have previously?

Now 1.0.0 has been released, it is my intention to release more regularly, especially when there are notable changes. What with WPCS using semantic versioning, releasing new versions should be easier now as the versioning communicates the impact of the release.

Sure, it should probably throw there, too.

Would you mind opening an issue in WPCS for this ? This is a significant change, so I'd like to see some responses from the community before making it, though I personally fully support this change.

[pento]

Replying to jrf:

That's what the composer.lock file is for.... or am I missing something ?

I'm a dummy, I forgot why we had to do that in Gutenberg: we were working with versions of npm that didn't support package-lock.json, so had to lock the versions ourselves. We can use composer.lock here.

Now 1.0.0 has been released, it is my intention to release more regularly, especially when there are notable changes. What with WPCS using semantic versioning, releasing new versions should be easier now as the versioning communicates the impact of the release.

Cool, let's try and get a release out with the customisations we currently have in phpcs.xml.dist. I don't mind a process where we make tweaks in Core's phpcs.xml.dist, then remove them when they're moved to WPCS, instead.

Would you mind opening an issue in WPCS for this ? This is a significant change, so I'd like to see some responses from the community before making it, though I personally fully support this change.

Is ​WPCS#1323 sufficient?

[jrf]

Cool, let's try and get a release out with the customisations we currently have in phpcs.xml.dist.

For now, that would just mean adding the WordPress.CodeAnalysis.EmptyStatement sniff to WordPress-Core and the additional property for the PEAR.Functions.FunctionCallSignature sniff.

All the excludes should stay in the custom ruleset.

I don't mind a process where we make tweaks in Core's phpcs.xml.dist, then remove them when they're moved to WPCS, instead.

That's fine by me, but let's make sure that for each tweak in the custom ruleset an issue is opened in the WPCS repo to keep WPCS moving along.

Is ​WPCS#1323 sufficient?

​WPCS#1323 actually argues *against* adding the property. It also highlights a problem in the documentation generator which needs to be fixed (or maybe it has been since ?).

[pento]

Okay, partially addresses ​WPCS#1330, then. 🙂

I don't know if the docs parser will handle this change, I asked on #43644, but didn't get a response.

@johnbillion: Can you please confirm if the docs parser can manage multi-line action calls, where each parameter is on a new line? eg:

do_action(
        'upgrader_process_complete',
        $this,
        array(
                'action' => 'update',
                'type'   => 'core',
        )
);

[azaozz]

Just pointing out that I'm (still) very much against encouraging multi-line parameters that are multi-line themselves. The above example looks "cute":

do_action(
        'upgrader_process_complete',
        $this,
        array(
                'action' => 'update',
                'type'   => 'core',
        )
);

But in reality we are "approving" stuff like this:

$content = apply_filters(
        'wp_comment_no_reply',        // First arg.
        '',                           // Second arg.
        array(                        // Third arg.
                'position' => $position,
                'checkbox' => $checkbox,
                'mode'     => $mode,
        ),
        esc_html(                     // Forth arg.
                sprintf(              // Forth.one arg.
                        __( 'This is a long piece of text that should never be inside of a function call. Even worse: it might end up in a nested multi-line function call. %s, %s.' ),
                        add_query_arg(   // Forth.one.two arg.
                                array(   // Forth.one.two.one arg.
                                        'bah_where_am_i'     => 'undefined', // Forth.one.two.one.one arg.
                                        'how_did_i_get_here' => 'who-knows', // Forth.one.two.one.two arg.
                                ),
                                remove_query_arg( // Ughhh, getting worse and worse. Forth.one.three arg.
                                        array(
                                                '_wp_http_referer', // Forth.one.three.one.one arg.
                                                '_wpnonce', // I give up :)
                                                'error',
                                                'message',
                                                'paged',
                                        ),
                                        wp_unslash( $_SERVER['REQUEST_URI'] )
                                 )
                        ),
                        '<p>' . __( 'This is another long piece of text that should never be inside of a function call...' ) . '</p>'
                )
        )
);

IMHO this is bad/ugly way to write. It is hard to read with many readability problems:

• Many levels of unneeded indentation.
• No (good) way to add inline docs. Adding few docblocks will make this even harder to read.
• No good way to even add simple inline comments, see above.

I understand this is difficult (impossible?) to fix with the parser. Thinking to amend the core coding standards to strongly discourage such code, even only as a recommendation.

[jrf]

I understand this is difficult (impossible?) to fix with the parser. Thinking to amend the core coding standards to strongly discourage such code, even only as a recommendation.

Correct, this is impossible to auto-fix, though it will be relatively easy to write a sniff to warn against multi-line parameters (line count > param count), but how to handle function calls with inline comments ?

Think:

<?php
printf(
    /* translators: 1: explanation; 2: explanation. */
    esc_html__('Text %1$s with %2$s replacements'),
    $replacement1,
    $replacement2
);

Disregard lines which only contain whitespace and a comment completely ?

What about completely blank lines in function calls ?

Things like this will need to be discussed in the WPCS issue before such a sniff could be written.

[azaozz]

Replying to jrf:

Disregard lines which only contain whitespace and a comment completely ?

Yeah, think so.

What about completely blank lines in function calls ?

Thinking there shouldn't be any blank lines there, generally. Perhaps in some edge cases (a "monster" functions with ten parameters each on a new line) blank lines followed by inline comments may be acceptable. But don't see a problem to warn about these as well.

Things like will need to be discussed in the WPCS issue before such a sniff could be written.

Of course. Lets look at that after the core coding standards are amended.

[pento]

Replying to jrf:

Correct, this is impossible to auto-fix, though it will be relatively easy to write a sniff to warn against multi-line parameters (line count > param count), but how to handle function calls with inline comments ?

Would it be possible to write a sniff for param line count > 1? It seems like that would take care of the cases where inline comments or blank lines exist inside the function call.

[jrf]

@pento

Let's move the implementation discussion to the WPCS ticket please.

Would it be possible to write a sniff for param line count > 1? It seems like that would take care of the cases where inline comments or blank lines exist inside the function call.

The determination is not a problem, the "should an error/warning be thrown" bit, is.

[pento]

In 43571:

Coding Standards: Upgrade WPCS to 1.0.0

WPCS 1.0.0 includes a bunch of new auto-fixers, which drops the number of coding standards issues across WordPress significantly. Prior to running the auto-fixers, there were 15,312 issues detected. With this commit, we now drop to 4,769 issues.

This change includes three notable additions:

• Multiline function calls must now put each parameter on a new line.
• Auto-formatting files is now part of the grunt precommit script.
• Auto-fixable coding standards issues will now cause Travis failures.

Fixes #44600.

[jrf]

Would it be an idea to put WPCS on the agenda for a core chat in the near future ? And if so, which core chat would be the best one to use ?
Or should there maybe be a dedicated bi-monthly Core CS chat ?

Aside from the above mentioned ruleset changes - which are basically a foregone conclusion -, there are a number of other sniffs in the WordPress-Extra ruleset which are good candidates to be moved to the WordPress-Core ruleset and some issues which could use some more discussion before a sniff can be written for them.

We could also discuss how to bring the non-autofixable issues down further in such a chat.

[pento]

Let's schedule a Core CS chat, we can mention that it's happening in the Core chat, so folks are aware.

I agree that it'd be valuable to go through WordPress-Extra and see what we can move to WordPress-Core, as well as any other discussion that comes up.

[jorbin]

In 43684:

Build/Test: Update dependencies for 5.0

Updates package dependencies to more modern versions, also adds .nvmrc and package-lock.json as followups to [43683] and [43571].

See #44600.
Fixes #45064.

[netweb]

Replying to pento:

• Create a Grunt precommit task to run composer format on any changed PHP files.

As part of adding PHPCS to the 5.0 branch reopening and moving back to the 5.0 milestone to add the above Grunt task

[pento]

Moving back to 5.1 and re-closing, as this isn't going to make it into 5.0.

[atimmer]

In 43977:

Build/Test: Update dependencies for 5.0

Updates package dependencies to more modern versions, also adds .nvmrc and package-lock.json as followups to [43683] and [43571].

Merge notes: trunk already had an identical .nvmrc. package-lock.json exists in trunk, but has some changes based on the dependency updates.

Props jorbin.
Merges [43684], [43685] and [43686] to trunk.
See #44600.
Fixes #45064.