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.

Note that adding type hints to existing code might break backward compatibility. For this reason it is allowed for code that needs to retain backward compatibility to omit type hints.

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 an appropriate interface is available. If specifying a type, Always specify an Interface. That allows other developers to provide their own implementations if necessary without modifying existing code.
  • Implementation note: 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.
  • Adding type hints to existing code might break backward compatibility. For this reason it is allowed for code that needs to retain backward compatibility to omit type hints.

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.

tizzo’s picture

Issue summary: View changes

Updating the issue summary conform with the most recent suggestions.

tizzo’s picture

Issue summary: View changes
tizzo’s picture

Issue summary: View changes
pfrenssen’s picture

This looks great to me. +1!

effulgentsia’s picture

Project: Coding Standards » Drupal core
Version: » 8.2.x-dev
Component: Documentation » other
Issue tags: -Coding standards, -final discussion +Approved Coding Standard

Great! Looks like pretty good consensus here, so moving to the core queue for core committers to decide on when to implement, per step 7 of https://www.drupal.org/project/coding_standards. I suspect core still has lots of places where we're not currently typehinting array.

fgm’s picture

It seems this leaves the case of result type declarations (function foo(): string) not covered, while this is very much the same issue. Need it be another issue, or could it be bundled in this one ?

xjm’s picture

Title: Add type hinting to function declaration coding standards » [policy, no patch] Add parameter type hinting to function declaration coding standards
Status: Reviewed & tested by the community » Needs review

@fgm, separate discussion I think.

@alexpott and I brainstormed a bit and there is really no way to test this rule on its own with coder at least until PHP 7 is our minimum version and scalars can be typehinted too, because there's no way coder can deduce what the allowed inputs of the function should be and whether they are scalar or not.

So, the next step for this issue would be to determine the scope of the work needed for Drupal core to comply, in order to determine whether this rule can be adopted for D8 with a minor release. I think we already have this expectation somewhat, so hopefully that would make the task less daunting, but I think we need some research to figure out how well core follows this.

A strategy I'd suggest is comparing with documentation typehints. Those typehints are probably not present everywhere (see #2571965: [meta] Fix PHP coding standards in core for work to make core compliant with that and other existing rules), but where they are array or contain a backslash, that's a good sign that the corresponding parameter needs a typehint in the declaration.

We might also want to postpone making core comply with this rule on being compliant with the documentation typehint issue, to make it easier to validate. Determining the correct allowed type for parameters actually requires careful work. Since the doc typehints also apply to scalars, we can ensure compliance with that first, and then we could programmatically check the parameter typehint against the documentation typehint to ensure all non-scalars were typehinted and that the types matched.

fgm’s picture

I would be tempted to say that we could at least allow result typing in the standards (last I checked, it rejects it, even if a module requires PHP 7.x), without actually checking the result type in coder : this would likely only require a minor (tokenizer) change, not full parsing and interpreting.

Of course, in that case the value is reduced, but it does not contradict later changes to mandate it, and allows forward-looking devs to already include it.

David_Rothstein’s picture

For new code, Drupal 9 code, etc., the proposal in this issue is fine.

However making this change for existing code is much more than just a coding standards issue. PHP throws a fatal error when you pass in something to a function that violates the type-hinting, so what that means in practice is that changing this for existing code would result in lots of "The website encountered an unexpected error. Please try again later" error pages on people's live sites. (See #1279680: watchdog() does not type its array arguments for some previous discussion about that.)

So I think this needs more discussion on how to handle this for existing code. I don't think we can make public API functions in core start adhering to this new standard in the middle of a stable release.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

effulgentsia’s picture

So I think this needs more discussion on how to handle this for existing code.

I opened #2788295: How to handle and document deprecated/versioned coding standards?.

pfrenssen’s picture

Issue summary: View changes

I've updated the proposal with clear warnings that this change might affect backwards compatibility and an exception is allowed for old code:

Note that adding type hints to existing code might break backwards compatibility. For this reason it is allowed for old code to omit type hints.

effulgentsia’s picture

Issue summary: View changes

I like #41. I tweaked it to s/old code/code that needs to retain backward compatibility/.

pfrenssen’s picture

This now needs an issue in Coder to add a rule for this. This was already part of Coder but was removed in #2560651: PHP_Codesniffer flagging 'Type hint "array" missing...' but type hints not a standard. In the new issue we can have a look if it is still possible to revert this patch.

klausi’s picture

The Coder issue to require type hints for arrays if @param says "array" has been implemented: #2803251: Restore the rule to require array type hints

Liam Morland’s picture

What about the opposite? If the array type hint is given, does it raise an error if the @param has anything other than "array" as the type?

drunken monkey’s picture

What about the opposite? If the array type hint is given, does it raise an error if the @param has anything other than "array" as the type?

That wouldn't be correct, since it could also be something like \Drupal\SomeInterface[].
Same in the other direction, though – seems like Coder currently doesn't check for that either?
(Third option: If the parameter can also be NULL, Coder should probably also check that this is correctly documented?)

Liam Morland’s picture

OK, but still if the array hint is given, then the @param needs to be "array" or end with "[]".

If the function signature has "array $foo = null", then the @param should have "array|null".

klausi’s picture

Yes, Coder covers this already, please test.

Liam Morland’s picture

Yes, it is working for me.

effulgentsia’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC to see if #36 has been sufficiently addressed.

xjm’s picture

Status: Reviewed & tested by the community » Needs work

This has not been addressed:

So, the next step for this issue would be to determine the scope of the work needed for Drupal core to comply, in order to determine whether this rule can be adopted for D8 with a minor release. I think we already have this expectation somewhat, so hopefully that would make the task less daunting, but I think we need some research to figure out how well core follows this.

xjm’s picture

I also think #38 is important here and depending on the scope of core's compliance (or not) opening #2788295: How to handle and document deprecated/versioned coding standards? might not be sufficient to address it for this issue.

effulgentsia’s picture

Re #52: #41 updated this issue's IS to address #38.

effulgentsia’s picture

Tagging this issue for a summary update to provide the information requested in #51.

Evgeny_Yudkin’s picture

Think thats pretty necessary moment, it moves together with modern php trends.

effulgentsia’s picture

This part of the issue summary is confusing, and the wording could/should be improved prior to updating the official coding standards:

DO NOT use a class as the type in type hinting for objects if an appropriate interface is available. Always specify an Interface.

First we say if. Then we say always. I think that's confusing. As just one example, #2835758: Remove BigPipeInterface and move all of its docs to the implementation is an issue filed today to remove an interface, and change the corresponding type hints to the implementation class. I think we have more of these kinds of cases in core, and they're legitimate for cases like this, where we don't want to promise stability of the interface. So, I think the if is correct, and the always is incorrect.

effulgentsia’s picture

#56 makes a Coder rule for that portion impossible, I think. How can Coder know if an appropriate interface is available? I suppose maybe it could check for the presence of a PHP file with the same name as the type hint with Interface.php suffixed? Sometimes the appropriate interface doesn't follow that naming convention, but this would at least catch the ones that do.

effulgentsia’s picture

Additionally, perhaps it could check whether the type hinted class implements any interfaces not following that naming convention, and issue a warning for whether to change the type hint to one of them?

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

queenielow’s picture

Hi,

I'm not sure if this is the right place to post my findings. I'm sorry if this is completely irrelevant.

I was using hook_form_alter(&$form, \Drupal\Core\Form\FormStateInterface $form_state, $form_id) for my custom module and found that it does not match the code sniffer code standards.

The message return was :

  • Type hint "array" missing for $form
  • Expected type hint "array"; found "FormStateInterface" for $form_State

If I were to add array on the hook, it will break the code as Drupal expected $form and $form_state as an object.

In order to get around the code sniffers, I have to add these code to my function so that both sides are happy.
// @codingStandardsIgnoreStart and // @codingStandardsIgnoreEnd

Is there a way we could integrate with Drupal function standard or better workaround?

Cheers,
Queenie

fgm’s picture

You probably need to use an up-to-date sniffer configuration, maybe following the installation instructions on https://drupal.org/project/coder about the configuration of phpcs for Drupal. Your form alter should look like:

/**
 * Implements hook_form_alter().
 */
function foo_form_alter(array &$form, FormStateInterface $form_state) { 
  // some code
}

Points to note:

* no @param on hook implementations
* no fully qualified class names on hints: classes need to be imported

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

larowlan’s picture

Is this the best place to discuss support for scalar type-hinting now that Drupal 8/9 require at least PHP 7 or more?

Or should we start a separate conversation?

xjm’s picture

Now that Drupal no longer supports PHP 5 even for backports and Drupal 9 is nearly ready to ship, I think we should adopt a best practice of adding scalar typehints to new APIs.

A somewhat trickier problem is how to add new signature typehints for existing APIs without breaking BC and/or without having to deprecate half of core. (This applies especially for the scalar typehints, but also for the occasional missed typehint for an array or object. I had a list of issues related to missing typehints that I seem to have misplaced, but there's places in core where we're missing typehints already.)

Liam Morland’s picture

Perhaps this should be a goal of Drupal 9.1. Add type declarations everywhere and during the development period any issues caused by this can be found and fixed. Fixing such issues would rarely be more than adding a cast where a function is called.

drunken monkey’s picture

Perhaps this should be a goal of Drupal 9.1. Add type declarations everywhere and during the development period any issues caused by this can be found and fixed. Fixing such issues would rarely be more than adding a cast where a function is called.

It’s not about the calls, it’s about overridden methods in child classes. When we add a typehint, overriding methods would need to choose whether they want to be compatible with Drupal 9.0 or Drupal 9.1.

However, the situation is at least a bit better in Drupal 9, as PHP 7.2 introduced proper covariance/contravariance for return/parameter type hints. So when we’d add new typehints to existing methods after Drupal 8.9 has been retired, modules would at least be able to support all active Drupal versions simultaneously, even if it wouldn’t be ideal. (I.e., they’d add the return type right away, but refrain from adding the parameter type hints until dropping support for the older Drupal version.) Adding a new return type hint would still immediately break all modules that include an override for the method/function in question, but they would be easy to fix, and adding a new parameter type hint would even be completely safe from this standpoint.

So, maybe this could indeed be a way to getting proper type hints in place for existing code? It would still be pretty disruptive, I fear, but probably the best option we have, if we judge it’s worth it. (It probably isn’t, though.)

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

andypost’s picture

The issue becomes more viable in context of PHP 8.1
details in #3224523: [PHP 8.1] Add ReturnTypeWillChange attribute where necessary

andypost’s picture

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.