Problem/Motivation

Running phpcs after installing coder (via composer). Seeing many errors like this:

 567 | ERROR | Type hint "array" missing for $element
 567 | ERROR | Type hint "array" missing for $form_state

for function declarations like this one 

/**
 * Form validation function to see if product id already exists.
 *
 * @param array $element
 *   Form element to validate.
 * @param array $form_state
 *   Form state object.
 */
function bv_form_product_id_validate($element, $form_state) {
...
}

Type hinting is not, as far as I can tell, a current Drupal coding standard for function declarations (see https://www.drupal.org/coding-standards#functdecl), so these errors seem to be incorrect - running phpcs --standard=Drupal should not flag them.

These errors have been replicated on two different systems - Mac OS X Yosemite and Ubuntu 10.0.4

Mac:

DevelopmentsMBP:blink_jjbos jeff$ phpcs --version;php --version;composer global info --installed
PHP_CodeSniffer version 2.3.3 (stable) by Squiz (http://www.squiz.net)
PHP 5.5.27 (cli) (built: Jul 23 2015 00:21:59) 
Copyright (c) 1997-2015 The PHP Group
Zend Engine v2.5.0, Copyright (c) 1998-2015 Zend Technologies
Changed current directory to /Users/jeff/.composer
drupal/coder              8.2.3              Coder is a library to review D...
squizlabs/php_codesniffer 2.3.3              PHP_CodeSniffer tokenizes PHP,...

Ubuntu:

jeffmarkel@swizzle:~$ phpcs --version;php --version;composer global info --installed
PHP_CodeSniffer version 2.3.3 (stable) by Squiz (http://www.squiz.net)
PHP 5.3.2-1ubuntu4 with Suhosin-Patch (cli) (built: Apr  9 2010 08:23:39) 
Copyright (c) 1997-2009 The PHP Group
Zend Engine v2.3.0, Copyright (c) 1998-2010 Zend Technologies
    with Xdebug v2.0.5, Copyright (c) 2002-2008, by Derick Rethans
Changed current directory to /home/jeffmarkel/.composer
drupal/coder              8.2.3 Coder is a library to review Drupal code.
squizlabs/php_codesniffer 2.3.3 PHP_CodeSniffer tokenizes PHP, JavaScript and CSS files and detects violations of a defined se...

Proposed resolution

Remove code which tests for type hints and subsequently returns false positives.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#2 coder-php_codesniffer-typehint-false-positives-2560651-2.patch1.23 KBjmarkel

Comments

jmarkel created an issue. See original summary.

jmarkel’s picture

jmarkel commented
Status: Active » Needs review
StatusFileSize
new coder-php_codesniffer-typehint-false-positives-2560651-2.patch1.23 KB

Here's a patch which removes the section of code that checks for and returns the false positives 'Type hint' errors.

jmarkel’s picture

jmarkel commented
Issue summary: View changes

  • klausi committed 9d2ba49 on 8.x-2.x 
    Issue #2560651: do not enforce array type hints, not in our coding...
klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Version: 8.x-2.3 » 8.x-2.x-dev
Status: Needs review » Fixed

I committed a fix that does not throw this error when arrays are used. For classed objects we have in our object oriented coding standards:

DO specify a type when conformity to a specific interface is an assumption made by the function or method. Specifying the required interface makes debugging easier as passing in a bad value will give a more useful error message.

https://www.drupal.org/node/608152#hinting

So using type hints in general is encouraged and I think we should keep this sniff that if you document a parameter as a class/interface you should also use that class/interface as type hint.

pfrenssen’s picture

pfrenssen
Location Sofia
commented

This is a de facto standard. I have seen many reviews for D8 core issues in which it was pointed out that a type hint for an array was missing. I just did a grep of core and there are 5500+ instances where this type hint is used.

Even more important, if your argument is declared as an array and the type hint is not there then you are missing out on a very helpful mechanism in PHP that will help you discover bugs. If you expect an array and get a different type then you should be warned about this. You could even argue that omitting this type hint is a bug in itself.

I cannot say how many times this sniff has helped me finding bugs in my code and I'm sad to see that it has been removed. Can we revert this, or at least have it throw a warning instead of an error?

I will also open an issue to have this formally added to the coding standards.

pfrenssen’s picture

pfrenssen
Location Sofia
commented
Status: Fixed » Active

There is already an issue for adding array type hints to the coding standards: #1158720: Improve text for parameter type hinting in function declaration. In that issue @YesCT, @jhodgdon and @chx (by proxy) agree on adding it, and nobody is against. It seems it is just a matter of actually writing the documentation.

Quoting from that issue:

from conversation with @chx in irc:

We do not type hint primitive types. you can't scalar hint with php
We always hint if we can -- arrays and objects.
But if it can be NULL, we do not type hint because php is ... wont let us.

I've found adding type hints to patches sometimes exposes where the comments in the doc block @param was wrong. That sometimes it whatever or NULL, or it's mixed type, etc.

So type hinting seems to be a good thing.

Reopening this for discussion.

jmarkel’s picture

jmarkel commented

Re #6 and #7, I agree that this should be considered as an addition to the standard. it's been floating around as a proposal for at least 4 years, but to date the proposal is no more than that - a proposal. But the rules embodied in Coder are, or should be, a reflection of the standards that are, not the ones that should be, so the discussion should be taking place in #1158720: Improve text for parameter type hinting in function declaration. If consensus is reached there and the standards are changed, that's when this sniff should be re-added.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Active » Fixed

Agreed, let's revisit this once the standard has been changed.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented

This has now been reverted in #2803251: Restore the rule to require array type hints because we will require type hints in the coding standards.

greg boggs’s picture

greg boggs
he/him
Primary language English
Location Portland Oregon
commented

This error is back, I think? I have a type hint for an array and getting this error again:

  /**
   * Custom html block.
   *
   * @param array $array
   *   Settings array.
   * @param string $title
   *   Title string.
   * @param string $string
   *   Sharethis path.
   *
   * @return array
   *   Array with options, title and path.
   */

79 | ERROR | Type hint "array" missing for $array

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented

Do you have "array" as type hint on the function signature? Or is it missing there?

admirernepali’s picture

admirernepali commented

The error is happening for me too:

/**
 * Prepares variables templates.
 *
 * Default template: hook.html.twig.
 *
 * @param array $variables
 *   An associative array containing:
 *   - view: A ViewExecutable object.
 *   - rows: The raw row data.
 */
function template_preprocess_hook(&$variables) {