Feedback on initial setup

[jrfnl]

Hi @soderlind, as discussed on Twitter, please find some initial feedback below:

The most problematic issue with this standard is the non-conformation to the naming rules set out by PHPCS.
This means that:

• Refering to the sniff name (in contrast to the standard name) in a custom ruleset or from the command line will not work.
• Setting properties from a custom ruleset won't work.
• Whitelisting code using the selective ignores // phpcs:ignore StandardName or more selectively // phpcs:ignore StandardName.Category.SniffName -- for reasons won't work.
  etc

See my remarks below on how to fix this.

Repo name:

There are many repos/packages called coding-standard.

1. The name is not descriptive.
2. The name is not correct. This is not a (complete) coding standard, this is an external PHPCS standard checking for one specific thing.

Consider naming the package/repo phpcs-fqn-global-functions or something. That would be a lot more descriptive.

As a side-note: it will also prevent issues for people contributing to multiple CS standards repos getting a conflict notice when cloning (yet another) repo called coding-standard.

composer.json

• Missing squizlabs/php_codesniffer as a dependency.
  This means that there is no reliable information about with which PHPCS version the standard is supposed to be compatible.
  The fact that the DealerDirect plugin includes PHPCS is irrelevant as the PHPCS versions they support - "^2|^3" - may vary significantly from what you support for this standard/sniff.
• Please adjust the dealerdirect/phpcodesniffer-composer-installer requirement to:
  1. Be more flexible to prevent conflicts with other external standards - see: https://github.com/Dealerdirect/phpcodesniffer-composer-installer/#requiring-the-plugin-from-within-your-coding-standard
  2. Include the latest minor 0.6.2.
     Composer treats minors below 1.0 as majors, so will not allow any of the 0.6.x range to be installed now.

README.md

• Why?: => Why?
• Install: consider mentioning that the DealerDirect Composer PHPCS plugin will be installed and will register the sniff with PHPCS automatically.
  This should preempt support questions about this.
• The example ruleset code is invalid.
  Please validate it against the PHPCS XSD schema.
  • <rule name="FullyQualifiedGlobalFunctions"> should be <rule ref="FullyQualifiedGlobalFunctions">.
  • <description> is not a valid element within a <rule> tag. Use a XML comment instead.
  • Properties should be set for individual sniffs, not for the complete ruleset.
  • Spelling: <!-- whether to add backslash to all gobal functions or only optimized global funtions --> =>
    <!-- Whether to add a backslash to all global functions or only optimized global functions. -->
• Regarding the :red_medium_square: remark: correct. This has to do with the invalid namespace, see remarks below on how to fix this.

ruleset.xml

• The namespace used is invalid and breaks the PHPCS native autoloader.
  Either the attribute should be removed and the main namespace should be FullyQualifiedGlobalFunctions.
  Or the namespace should end with that, like Soderlind\FullyQualifiedGlobalFunctions.
  This also needs adjusting in the sniff.
  Also see: PHPCS 3.x: Autoloading not working with arbitrary namespace prefix squizlabs/PHP_CodeSniffer#1469
• Don't set properties for your own sniff in the ruleset. Set them as the default property value for the sniff instead.
• Please add the XSD to the ruleset.

The actual sniff file

• Sniffs need to be placed in a category sub-directory, not directly in the Sniffs directory.
• The namespace needs to reflect both the category as well as what I said above about the namespace.
• declare( strict_types = 1 ); and a return type of void are being used. This makes the minimum PHP requirement of the standard different from the PHPCS native minimum requirement, i.e. the PHP requirement needs to be added to the composer.json require section.
• The property which is intended for people to change from the ruleset - $onlyOptimizedFunctions - should be public. PHPCS will then automatically adjust the value when it reads the ruleset.
  The ruleset reading code in get_ruleset_property() can also be removed once the property has been made public (as well as the $sniff property).
  Also: sniff properties can be changed during a run - particularly for unit tests -, so don't presume one value for all scanned code. This also means that this code $this->globalFunctions = $this->optimizedFunctions will need changing.
• As the namespace is wrong, the PHPCS autoloading is broken, which means that matching properties to sniffs will not work, i.e. the public property is unusable.
• List of $optimizedFunctions is missing sizeof.
• $tokens[ $stackPtr ]['code'] === T_STRING: The sniff will only be called on T_STRING tokens, so this never needs to be checked. That's what the register() method is about.
• The code to check whether something is a function call is flawed.
  • Whitespace and comments in unexpected places will be ignored by the PHP parser, so the sniff should ignore them too.
  • findPrevious() and findNext() can return false. As that's not being checked, the sniff can throw undefined index notices now.
  • The whole if ( isset( $ignore[ $tokens[ $nextToken ]['code'] ] ) === true ) is redundant. See the next condition.
  • What about use function statements ? including those with aliases ?
  • What about functions returning by reference ?
• What about user defined global functions ?
• Code readability: false !== isset( $this->globalFunctions[ $function ] ) should be either isset( $this->globalFunctions[ $function ] ) or true === isset( $this->globalFunctions[ $function ] ).
  That check should probably be moved up as it is an early exit before doing the more expensive token walking.
• The PHPCS add[Fixable]Error() function already has sprintf() build in, so no need to do it yourself. Just pass the replacements as an array in the fourth parameter.
• The errorcode used is kind of useless as it repeats the sniff name and makes the sniff code unnecessarily long. I'd suggest something simpler like NotFullyQualified.
• As part of the fix the case of the function call is being changed if not lowercase to begin with. That is not the responsibility of this sniff. The original content should be used in the fix.
• Why have the one line fix() function ? It's only being called from one place in the code-flow, so it doesn't prevent duplicate code and it causes (marginal) overhead due to the extra function call + need for the $fixer and $stackPtr properties, which wouldn't be necessary.
• I'd suggest removing the --runtime-set onlyOptimizedFunctions true option. That is not really intended for properties.
  There are use-cases for this, but I don't see any reason why this sniff should have that option.

Other

• I'd like to suggest having a good think of what PHPCS versions the standard should support. Depending on the version, there may be quite some extra things to take into account.
• Adding some unit tests would probably be a good idea, including testing the property ;-)
• Adding automated CI as well.

You may also find it useful to read through the feedback I gave for another standard: Rarst/phpcs-cognitive-complexity#1

Other useful references:

• https://github.com/squizlabs/PHP_CodeSniffer/wiki/Coding-Standard-Tutorial
• Custom rule must end in "Sniff"?? squizlabs/PHP_CodeSniffer#2481 (comment)
• "ERROR: Referenced sniff does not exist" while trying to include custom sniff squizlabs/PHP_CodeSniffer#2606