I think we should change the "Function Declarations" in the Coding standards page, so it advises people to use type hinting, when declaring functions that have arrays as arguments.

See more on: http://drupal.org/node/318016

The text could say:

"When the function takes an array as an argument, use type hinting."

And as an example we could use


function my_func(array $foo, $bar) {

} 

Proposed update to coding standards

Proposed update for the Function declaration section in the main coding standard page. The code example is updated as well as the sentence in bold text.

function funstuff_get_animal(HerdInterface $herd, array $options = []) {
  $name = !empty($options['animal_name']) ? $options['animal_name'] : 'Fido';
  return $herd->getAnimal($name);
}

Arguments with default values go at the end of the argument list. Always use type hints for the arguments. Always attempt to return a meaningful value from a function if one is appropriate.

Proposed update for the Type hinting section in the coding standards for object oriented code. Adding the sentence in bold text:

PHP supports optional type specification for function and method parameters for classes and arrays. Although called "type hinting" it does make a type required, as passing an object a parameter that does not conform to that type will result in a fatal error.

  • DO specify a type when conformity to a specific interface is an assumption made by the function or method type hints for all parameter types that support it. Specifying the required interface This makes debugging easier as passing in a bad value will give a more useful error message.
  • DO NOT use a class as the type in type hinting for objects. If specifying a type, Always specify an Interface. That allows other developers to provide their own implementations if necessary without modifying existing code.

Comments

jbrown’s picture

Title:Revise the section about "Function Declarations" in the Conding standards page.» Add type hinting to function declaration coding standards
arianek’s picture

Issue tags:-Type Hinting+developer

tags

jhodgdon’s picture

Project:Documentation» Drupal core
Version:» 8.x-dev
Component:Correction/Clarification» documentation
Assigned:johsw» Unassigned
Issue tags:-developer+coding standards

Normally we discuss coding standards in the Drupal Core issue queue, so moving there.

YesCT’s picture

I was looking around the coding standards and also doc standards, and could not find a reference to type hinting. But I thought we did type hinting except for strings.

YesCT’s picture

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.

jhodgdon’s picture

Title:Add type hinting to function declaration coding standards» [policy] Add type hinting to function declaration coding standards

we've been making titles like this for coding standards issues...

jhodgdon’s picture

Project:Drupal core» Drupal Technical Working Group
Version:8.0.x-dev»
Component:documentation» Documentation
Issue summary:View changes

Coding standards decisions are now supposed to be made by the TWG

tizzo’s picture

Project:Drupal Technical Working Group» Coding Standards
pfrenssen’s picture

There was already a sniff in the Coder module that was detecting if type hints were missing, but in #2560651: PHP_Codesniffer flagging 'Type hint "array" missing...' but type hints not a standard this sniff was removed with the argument that the coding standards do not require it.

I think omitting this type hint is not only a bad practice from a coding standards point of view but an actual bug, so I am 100% in favour of this being added to the standard.

pfrenssen’s picture

drunken monkey’s picture

Title:[policy] Add type hinting to function declaration coding standards» Add type hinting to function declaration coding standards

Also very much in favor of this! I think it's already more or less an unspoken law to use type hinting, and it does make a lot of sense – we should just make it official.

But if it can be NULL, we do not type hint because php is ... wont let us.

Luckily, this is not true (anymore?) – by just setting NULL as the default value for the parameter, you can add an "or NULL" option to your type hint.

PS: Since there is now an entire queue just for coding standards, I think the "[policy]" prefix isn't needed anymore?
And maybe we should use category "Plan" now, or define our own system for the categories?

pfrenssen’s picture

Proposed update for the Function declaration section in the main coding standard page. The code example is updated as well as the sentence in bold text.

function funstuff_get_animal(HerdInterface $herd, array $options = array()) {
  $name = !empty($options['animal_name']) ? $options['animal_name'] : 'Fido';
  return $herd->getAnimal($name);
}

Arguments with default values go at the end of the argument list. Always use type hints when possible. Always attempt to return a meaningful value from a function if one is appropriate.

Proposed update for the Type hinting section in the coding standards for object oriented code. Adding the sentence in bold text:

PHP supports optional type specification for function and method parameters for classes and arrays. Although called "type hinting" it does make a type required, as passing an object a parameter that does not conform to that type will result in a fatal error.

  • DO specify a type hint for arrays, and 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.
  • DO NOT use a class as the type in type hinting. If specifying a type, always specify an Interface. That allows other developers to provide their own implementations if necessary without modifying existing code.
Liam Morland’s picture

My feeling is that if the docblock has @param array $parameter, then the array type hint should be there. If you want to allow NULL, but can't set the default value for the parameter, then the docblock should have "array|null" and the type hint left out.

Results from PHP 5.6.17:

This gives an error:

function foo(array $test) {
}
foo(NULL);

This does not:

function foo(array $test = NULL) {
}
foo(NULL);
pfrenssen’s picture

@Liam Morland, very valid suggestion, and I agree this adds more clarity to the expected types. I would still provide a type hint in any case, omitting it just to save typing " = NULL" is not worth the additional risk that accompanies untyped parameters.

Do you think we should do the same for other types, like "string|null" and "int|null"?

I have done a search on Github and this pattern seems common in other frameworks as well, for example Symfony uses it, as well as Laravel and CakePHP.

pfrenssen’s picture

I just tested your code example on PHP 7.0.2, but it seems to work as designed. If a parameter is optional then you typically omit it, not pass NULL. For example this is the typical way of providing optional array parameters, and it works as expected if you omit the parameter:

function foo(array $test = array()) {
}

foo();
pfrenssen’s picture

Issue summary:View changes

Added the proposed change to the issue summary.

pfrenssen’s picture

Status:Active» Needs review
Xano’s picture

PHP supports optional type specification for function and method parameters for classes and arrays is outdated now PHP 7 supports scalar type hints as well. For clarity's sake, I think we should add this information here. Always use type hints when possible is not very helpful if you do not know much about type hints.

claudiu.cristea’s picture

function funstuff_get_animal(HerdInterface $herd, array $options = array()) {

As 8.x is the main release/branch, shouldn't we use the array short syntax for optional parameter default value?

pfrenssen’s picture

OK scalar type hints are indeed a reality now for PHP 7. It will be probably be a few years before we are going to see PHP 7 specific code popping up in core and contrib, but people are of course free to use it in custom project code.

I'll rewrite the proposal to be not specific about for which data types to use type hints, just that type hints are required. Then we are future proof.

The short array syntax is also a good suggestion.

pfrenssen’s picture

Issue summary:View changes
claudiu.cristea’s picture

Status:Needs review» Reviewed & tested by the community

Looks good to me. RTBC for version from #21.

tizzo’s picture

Issue tags:+final discussion
alexpott’s picture

Given the differences between PHP7 and PHP5 do we also mot need to say that typehints should only be those that owrk in the minimum PHP version? I'm not sure that future proofing here is a good idea.

Xano’s picture

The minimum PHP version for core may be different than the one for contributed modules.

Crell’s picture

If writing code that relies on PHP 7, then yes the same logic should apply: Type all the things, and I'd argue for strict mode as well for most files. But yes, we can't require PHP 7 yet. I would suggest adding the following parenthetical after "type hints for all parameter types that support it.":

(Note that PHP 7 adds additional type hints for scalar values. Those should be used if and only if the module is requiring PHP 7 for other reasons. Do not require PHP 7 solely for scalar types, but if a module does require PHP 7 their use is encouraged.)

xjm’s picture

I'd suggest changing it to "DO NOT use a class as the type in type hinting for objects if an appropriate interface is available" and adjusting the rest to match. The current wording implies that a class without an interface is never acceptable, and that's not actually true (nor within the scope of this standard to dictate).

Xano’s picture

The current wording implies that a class without an interface is never acceptable, and that's not actually true (nor within the scope of this standard to dictate).

+1

Crell’s picture

+1 to #27.