humanmade / coding-standards Goto Github PK

Directory names do not account for camel case or underscores in namespaces.

I have the file mu-plugins/pwcc-multi-domain/inc/post-types/namespace.php

• The namespace PWCC\MultiDomain\PostTypes throws an error requesting the directory name posttypes - report
• The namespace PWCC\MultiDomain\Post_Types throws an error requesting the directory name post_types - report

Oddly mu-plugins/pwcc-multi-domain/inc/namespace.php does not trigger an error for the namespace PWCC\MultiDomain.

The rule "Assignments must be first block of code on a line" reports incorrectly when using the following syntax:

$foo = $bar = 'thing';

Similar to wikimedia/mediawiki-tools-codesniffer@3a6709b

I think I might have seen this proposal somewhere before, but I can't find it here. Instead of requiring my_namespace/namespace.php where your namespace doesn't have any classes, we should support my_namespace.php so there's a little less folder cruft.

In order to better assess third-party code, we should split out the core (security, performance) rules from the style rules. We should do this for both PHP and JS, but PHP is a larger priority.

This sniff currently includes the token for interfaces, and errors, insisting that the file have a class prefix on the file name. I dont think this is correct behaviour a interface filename should be interface-foo.php not class-foo.php

Is this intentional or a bug, if its intentional whats the reasoning behind it.

The WordPress.WhiteSpace.ControlStructureSpacing.NoSpaceAfterCloseParenthesis rule is (currently) inconsistent.

I get:

Space between opening control structure and closing parenthesis is required
(WordPress.WhiteSpace.ControlStructureSpacing.NoSpaceAfterCloseParenthesis)

For:

function test2( array $foo ): \Generator {
    yield 1;
}

But no complaints for the exact same code without a function parameter:

function test1(): \Generator {
    yield 1;
}

It might be fixed upstream?

See: WordPress/WordPress-Coding-Standards#547 (comment)

We have installed the composer package in a WP plugin, this plugin has a package.json file with a dependency of react-scripts - v2.0.4.
The composer package has a dependency on 0.5.0 of this library.

When running eslint from the root of the plugin, we get the following error:

/Users/daniel-westall/sites/hmn.newsuk/vvv/www/wordpress-default/public_html/wp-content/plugins/nu-sun-web-wp-home-page-customiser-V2/vendor/humanmade/coding-standards/packages/eslint-config-humanmade/index.js:
    Configuration for rule "object-curly-newline" is invalid:
    Value "[object Object]" should be equal to one of the allowed values.
    Value "[object Object]" should NOT have additional properties.
    Value "[object Object]" should match exactly one schema in oneOf.
    Value "[object Object]" should NOT have additional properties.
    Value "[object Object]" should match exactly one schema in oneOf.

This version of react-script uses eslint v5. I couldn't find anything related to this rule specifically in the release notes
I'm guessing either the new version has some backward incompatible change that I haven't found, or there's something broken in the config of the plugin.

Also

[REACT] ./src/components/ItemClipboard.js
[REACT]   Line 1:  Definition for rule 'jsx-a11y/href-no-hash' was not found  jsx-a11y/href-no-hash

This rule seems to be disabled in our coding standards, so that shouldn't be happening

HM.Layout.Order.WrongOrder can throw a false positives in files with anonymous functions using use.

This code

<?php
namespace Hamilton;

use Hamilton\Mixtape;

const WRITER = 'Lin-Manuel Miranda';

add_filter( 'Hamilton/Cast', function ( $cast ) use ( $replacements ) {
	return $replacements;
} );

Error thrown:

----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 1 | ERROR | use found on line 8, but const was declared on line 6.
   |       | Statements should be ordered `namespace`, `use`, `const`,
   |       | `require`, then code. (HM.Layout.Order.WrongOrder)
----------------------------------------------------------------------

When using the HM Linter, you cannot extend the ESLint configuration by referring to the location of the ruleset on the filesystem.

Indeed it is sufficient to do the following (with sample rule customisation):

{
  "extends": "humanmade",
  "rules": {
    "react/react-in-jsx-scope": "off"
  }
}

As the Linter does not really have a documentation page, we should include these instructions on this repository instead.

If I run eslint --fix, the following code...

{ foo: 1, bar: 2 },

is converted into this.

{ 
    foo: 1, bar: 2,
},

I don't think we should allow this.

In this case I would expect this:

{ 
    foo: 1, 
    bar: 2,
},

file_get_contents() and other filesystem functions are fine. WP_Filesystem isn't appropriate 99% of the time.

Can we remove PSR2.Namespaces.UseDeclaration.MultipleDeclarations ("There must be one USE keyword per declaration")?

Modern PHP supports this syntax, and the above rule blocks it:

use some\namespace\{ClassA, ClassB, ClassC as C};
use function some\namespace\{fn_a, fn_b, fn_c};
use const some\namespace\{ConstA, ConstB, ConstC};

The NamespaceDirectoryNameSniff sniff currently requires namespaces within test directory files, but this should not be required as test files are outside of a plugin-based organizational structure.

Should check for /.test and /test directories in path and if found, bypass this rule.

We should include the rules from PHPCompatibility for PHP 7.2 in the base set of rules, to ensure all greenfields code is compatible for when we roll it out to servers.

The following code should be an error. The WP coding standards include checks for unescaped output WordPress.XSS.EscapeOutput.OutputNotEscaped

echo $foo;

But our coding standards are not flagging it - this rule is explicitly ignored

Was this intentional?

https://github.com/DealerDirect/phpcodesniffer-composer-installer is a better way of setting installed_paths than the hack we have. Should fix #2 in the process.

"wp-coding-standards/wpcs": "^0.10.0"
https://github.com/humanmade/coding-standards/blob/master/composer.json#L7

Should be 0.13.1:
https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/releases

Why?

Getting a lot of bogus errors when running Travis CI with PHP 7.2 (nightly)

Deprecated: Function create_function() is deprecated in /home/travis/build/[secure]/[secure]/vendor/wp-coding-standards/wpcs/WordPress/Sniffs/WP/PreparedSQLSniff.php 

Warning: count(): Parameter must be an array or an object that implements Countable in /home/travis/build/[secure]/[secure]/vendor/wp-coding-standards/wpcs/WordPress/Sniffs/WP/I18nSniff.php on line 234

...

Currently, we have a max-nesting rule that is very strict at only allowing 2 levels of nesting with scss, with the exception of blockless-at-rules. I think it might be beneficial to loosen this just a bit for @media rules by adding "ignoreAtRules": ["media"] to the general rule in Stylelint.

Stylelint currently already ignores @media at the first level in a nesting pattern when parsing for nesting depth, but it does not ignore @media when it resides down the chain. If we make the proposed change, @media levels will not affect the nesting depth evaluation, and we still shouldn't end up with too-specific of selectors.

Thoughts?

cc @peterwilsoncc @sambulance @goldenapples @rmccue @joemcgill @kirstyburgoine

The readme structure could be improved, by regrouping content.

I propose the following:

1. Intro paragraph
2. Setup
3. Contributing (former Testing). Located towards the top to encourage people to contribute.
4. Using PHPUnit (extend, ignore, how to find out which sniffs fail,...).
5. Using ESlint (same as above).

I'm unsure about the Included Checks section, because it could be interpreted as providing more background about the sniffs, or be considered a shout out section. We could integrate and extend this content based on what the goal is.

Let's discuss structure first, happy to follow up with a PR once we have settled on an outline.

We are currently whitelisting unit(less, actually)s for line-height only in our stylelint config, but I propose that we get more specific with what units are available.

Font Size

Common best practice is to use rem units for font size based off of a single root value. We should enforce rem units, or at a minimum, blacklist px units to prevent them from being used.

ms Units

For timing functions, such as transitions or animations, ms units are far clearer and easy to digest than seconds because they are laid out in longform. While they mean the same thing, 400ms is cleaner than .4s and feels more precise in my opinion. I suggest that we enforce ms units on all timing functions.

Ref https://stylelint.io/user-guide/rules/declaration-property-unit-whitelist/ & https://stylelint.io/user-guide/rules/declaration-property-unit-blacklist/

Thoughts #teamfrontend?

cc @peterwilsoncc @sambulance @goldenapples @rmccue @joemcgill

When submitting a pull request, Travis CI fails.

Current error:

0.17s$ cd /tmp/phpcs && composer install
Composer could not find a composer.json file in /tmp/phpcs
To initialize a project, please create a composer.json file as described in the https://getcomposer.org/ "Getting Started" section
The command "cd /tmp/phpcs && composer install" failed and exited with 1 during .

We should have real unit tests here.

We need to use ESLint so it correctly understands JSX.

These need to match the template hierarchy, so the name isn't flexible. We should exempt the following patterns from WordPress.Files.FileName.NotHyphenatedLowercase:

• category-*.php
• author-*.php
• taxonomy-*.php
• archive-*.php
• single-*.php
• page-*.php

The following are currently failing the object-curly-newline rules, and should not be:

import { Route, withRouter } from 'react-router-dom';
const { lastView, onDismiss } = props;

These appear to now have enforced newlines, when they should be optional.

Pretty sick of WPCS changing rules out from underneath us, which is due to specifying ^0.14.0. We should have a specific version instead.

In #41 we removed the object property alignment rule for JS, making this valid code:

const x = {
	foo: "bar",
	longerPropertyName: "value",
};

Should we do the same for PHP? Currently, this code fails:

$x = [
	'foo' => 'bar',
	'longerPropertyName' => 'value',
];

The error is:

Array double arrow not aligned correctly; expected 16 space(s) between "'foo'" and double arrow, but found 1.
(WordPress.Arrays.MultipleStatementAlignment.DoubleArrowNotAligned)

There's seems to be a new part of WordPress.Arrays.MultipleStatementAlignment.NotSameWarning that makes the following a failure:

$all['read_applications'] = true;
$all['read_application'] = true;

Do we want this / is this intentional? This is pretty dumb I think in most situations and probably leads to be putting lots of daniel spacing everywhere so the lines are not consecutive.

The following causes a warning: "There must be one USE keyword per declaration"

use function NewsUK\HomepageCustomiser\{get_layout_id_for_term,get_preview_script_url,get_preview_style_url,is_enabled_for_term};

This syntax is similar to how we write import statements in JS, so would make sense to allow

The following code is NOT flagged as an error, but I would expect it to be. Note missing space after the first (.

get_extended_template_part('atoms/heading', '4', [
	'heading_three' => 'This is a heading 3',
] );

However it is correctly enforced if the args are on a single line. e.g.

get_extended_template_part('atoms/heading', '4', [ 'heading_three' => 'This is a heading 3' ] );

http://engineering.hmn.md/style/ do not work. Instead http://engineering.hmn.md/

In particular, #8 introduced some more complex behaviour for the T_USE token.

This is done, because, I quote:

• A wrapper for PHP's parse_url() function that handles consistency in the return
• values across PHP versions.
• PHP 5.4.7 expanded parse_url()'s ability to handle non-absolute url's, including
• schemeless and relative url's with :// in the path. This function works around
• those limitations providing a standard output on PHP 5.2~5.4+.
• Secondly, across various PHP versions, schemeless URLs starting containing a ":"
• in the query are being handled inconsistently. This function works around those
• differences as well.
• Error suppression is used as prior to PHP 5.3.3, an E_WARNING would be generated
• when URL parsing failed.

As far as I can tell, parse_url() is safe to use if you know your environment is going to be PHP >= 7.0. Does this project have a minimum supported PHP version? If it does, and it matches this constraint, can we please consider adding a rule exclude for WordPress.WP.AlternativeFunctions.parse_url_parse_url thank you

We've had a Stylelint config sitting in this repo for almost a year now, and it would be be most useful to our project to release this and get it into practice on projects. This is a rough plan of steps involved with getting Stylelint moved forward and into use on our projects.

As with everything, would love perspectives and to know if I've missed a step here.

• Complete carry-on rule adjustments (#103, #106) cc @peterwilsoncc
• Add Lerna to repo
• Push Stylelint to npm as a standalone package, tagged 0.6
• Release 0.6 of full coding standards (we should focus on closing out open, completed PRs first)
• Include Stylint and any other carry-on changes as a 0.7 follow up to 0.6
• Write internal Dev post explaining Stylelint (what it is, how it works, how we'll release it)
• Add Stylelint to HM Linter (ref: humanmade/linter-bot#80)

The peer dependencies of our ESLint config need to be updated for ESLint 4:

warning " > [email protected]" has incorrect peer dependency "eslint@^3.8.1".
warning " > [email protected]" has incorrect peer dependency "eslint@^2.10.2 || 3.x".                                                                                    
warning " > [email protected]" has incorrect peer dependency "eslint@^2.0.0 || ^3.0.0".  

For React rendering performance we should consider adding jsx-no-bind to this eslint config; bound functions and arrow functions created within a render method's JSX will never be treated as equivalent when React is comparing subcomponent properties and can lead to a lot of unnecessary (and therefore expensive) re-renders.

This doesn't cause the behaviour we want, so we should remove it.

Possibly broken in #37.

• Inline PHP statement must end with a semicolon - the style guide specifically says not to do this

Currently we have:

space-unary-ops:
  - error
  - words: true
    nonwords: true
    overrides:
      ++: false
      --: false

and we should have:

space-unary-ops:
  - error
  - words: true
    nonwords: false

I think I saw some mention of a columns length change somewhere, but when I run phpcbf it's doing odd indenting / line returns, such as:

add_filter( 'determine_current_user', function ( $user ) {
	return 1;
} );

=>

add_filter(
	'determine_current_user', function ( $user ) {
		return 1;
	}
);

Like, what even is that?!

It seems console.error causes a eslint warning. Is this intentional? Is doing console.error / console.warn not a valid thing to do in production code?

There are currently errors on the master branch when running PHPUnit. This blocks the merge of any new PR's, as the merge button is flagged to only accept passing Travis runs. I'm opening this issue to track resolving these failures.

PHPUnit 5.7.27 by Sebastian Bergmann and contributors.
.......FFF.                                                       11 / 11 (100%)
Time: 140 ms, Memory: 12.00MB
There were 3 failures:

HM\CodingStandards\Tests\FixtureTests::test_passing_files with data set #1 ('/hm/gh/coding-standards/tests...er.php')

File /fixtures/pass/use-order.php should not contain any errors

Failed asserting that two arrays are equal.

--- Expected

+++ Actual

@@ @@

Array (


11 => Array (...)
15 => Array (...)
19 => Array (...)
23 => Array (...)
27 => Array (...)
31 => Array (...)
35 => Array (...)
39 => Array (...)
43 => Array (...)

)

/hm/gh/coding-standards/tests/FixtureTests.php:69

HM\CodingStandards\Tests\FixtureTests::test_passing_files with data set #2 ('/hm/gh/coding-standards/tests...ce.php')

File /fixtures/pass/inc/namespace.php should not contain any errors

Failed asserting that two arrays are equal.

--- Expected

+++ Actual

@@ @@

Array (


11 => Array (...)
13 => Array (...)
14 => Array (...)
15 => Array (...)
16 => Array (...)
18 => Array (...)
19 => Array (...)
21 => Array (...)
28 => Array (...)
30 => Array (...)
31 => Array (...)
32 => Array (...)
33 => Array (...)

)

/hm/gh/coding-standards/tests/FixtureTests.php:69

HM\CodingStandards\Tests\FixtureTests::test_failing_files with data set #0 ('/hm/gh/coding-standards/tests...er.php')

Failed asserting that two arrays are equal.

--- Expected

+++ Actual

@@ @@

Array (

1 => Array (...)


10 => Array (...)
16 => Array (...)

)

/hm/gh/coding-standards/tests/FixtureTests.php:118
FAILURES!

Tests: 11, Assertions: 8, Failures: 3.

Tested on Travis in #72 and locally with PHP 7.2.7

PHPCS 3.3 fixes an issue whereby the end of comment blocks was mis-identifed if a phpcs:disable/enable rule was included in the block. This is causing false issues in a client repo (ask me for more details if necessary). We should upgrade if there are not other various issues that arise form the upgrade.

See squizlabs/PHP_CodeSniffer#1918 for more details on the specific issue that I'me seeing.

We currently have no tests for the stylelint rules and we should add them to make sure that we're running the rules correctly and have greater confidence in changes.

I don't want to hold up #45 just for this, but we should add the tests in a subsequent PR.

Finding props sometimes in a React file is a bit difficult due to the random order. We could consider jsx-sort-props rule to automagically sort them alphabetically.

In the same line, we could also think about adding https://eslint.org/docs/rules/sort-imports to sort imports too.

In #75 @igmoweb proposed the addition of the sort-imports ESLint rule, to control the ordering of modules we import into our code.

In a Slack thread @rmccue further proposed that we conform to a general ordering of absolute, then relative, then side effects (e.g. importing a CSS file to ensure it gets included in the build, but not using the return of that import), with a blank line between each, and each section alphabetized.

As an example:

import React from 'react';
import { connect } from 'react-redux';

import Foo from './Foo';

import './Bar.css';

This made sense to me, and would help a lot on several projects we've had lately.

The sort-imports rule looks like it can handle putting the import-for-side-effects after other imports, but doesn't at first glance support including the relative imports after absolute. We should investigate further.

I have WordPress-VIP installed globally, and PHPCS can find it just fine:

$ phpcs -i
The installed coding standards are MySource, PEAR, PHPCS, PSR1, PSR2, Squiz, Zend, WordPress, WordPress-Core, WordPress-Docs, WordPress-Extra and WordPress-VIP

If I then install this repo globally, e.g.

$ composer global require humanmade/coding-standards
[snipped]
$ phpcs --config-set installed_paths /Users/leewillis/.composer/vendor/wp-coding-standards/wpcs/,/Users/leewillis/.composer/vendor/humanmade/coding-standards/
Config value "installed_paths" added successfully

HM shows up as an available standard:

$ phpcs -i
The installed coding standards are MySource, PEAR, PHPCS, PSR1, PSR2, Squiz, Zend, WordPress, WordPress-Core, WordPress-Docs, WordPress-Extra, WordPress-VIP and HM

However - it's unusable:

$ phpcs --standard=HM foo.php 

Fatal error: Uncaught PHP_CodeSniffer_Exception: Referenced sniff "WordPress-VIP" does not exist in /Users/leewillis/.composer/vendor/squizlabs/php_codesniffer/CodeSniffer.php:1167
Stack trace:
#0 /Users/leewillis/.composer/vendor/squizlabs/php_codesniffer/CodeSniffer.php(780): PHP_CodeSniffer->_expandRulesetReference(Object(SimpleXMLElement), '/Users/leewilli...', 0)
#1 /Users/leewillis/.composer/vendor/squizlabs/php_codesniffer/CodeSniffer.php(578): PHP_CodeSniffer->processRuleset('/Users/leewilli...')
#2 /Users/leewillis/.composer/vendor/squizlabs/php_codesniffer/CodeSniffer/CLI.php(956): PHP_CodeSniffer->initStandard(Array, Array, Array)
#3 /Users/leewillis/.composer/vendor/squizlabs/php_codesniffer/CodeSniffer/CLI.php(113): PHP_CodeSniffer_CLI->process()
#4 /Users/leewillis/.composer/vendor/squizlabs/php_codesniffer/scripts/phpcs(25): PHP_CodeSniffer_CLI->runphpcs()
#5 {main}
  thrown in /Users/leewillis/.composer/vendor/squizlabs/php_codesniffer/CodeSniffer.php on line 1167

I think because of the clobbering of install paths done in HM/ruleset.xml, but not sure what the fix would be.