Problem/Motivation

With PHP 7 we have primitive type hints, so now we can enforce better correctness of our code by using them. #3050720: [Meta] Implement strict typing in existing code discusses how to implement them, but first we should put a commitment into our coding standards that we want as many type hints as possible for at least all new code we add to Drupal.

Proposed resolution

Change https://www.drupal.org/docs/develop/standards/object-oriented-code#hinting , new text proposal:

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

  • DO specify a type hint for all function/method parameters and return values, if possible. That includes primitive type hints such as string, int, bool etc. Specifying the required type 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 possible). If specifying an object type, always specify an Interface. That allows other developers to provide their own implementations if necessary without modifying existing code.
  • Always add declare(strict_types=1); to the beginning of each PHP file to leverage strict type checks.

Example:

// Correct:
declare(strict_types=1);

function make_cat_speak(FelineInterface $cat, string $message): bool {
  print $cat->meow($message);
  return TRUE;
}

// Wrong:
function make_cat_speak(GarfieldTheCat $cat, $message) {
  print $cat->meow($message);
  return TRUE;
}

Remaining tasks

Build consensus that we want this and refine the proposed text here.

Comments

klausi created an issue. See original summary.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Issue summary: View changes

Adding return type hint example.

klausi’s picture

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

One approach to gradually enforce these new coding standards in Coder: if the file contains declare(strict_types=1); then we can check if each method/function in the file uses type hints, also for the return types. If the param/return docs specify "mixed" or multiple types with "|" then we can allow no type hint. That way we can still allow edge cases where people cannot use type hints.

A problem that remains is implementing interfaces that don't have type hints on a method. We could check if the doc comment is {@inheritdoc} and then not throw warnings if the type hint is missing.

The warning message in Coder could be "Type hint missing for parameter $x; if you need to leave it out then document the parameter as @param mixed or use {@inheritdoc} in case of interface implementations or method overrides"

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented

Generally I’m very +1 here.

If specifying a type, always specify an Interface.

Should probably be rephrased a little to make clear that this only applies to object types, and does not forbid hinting primitive types. (I know, should be clear from context, but still.)

Also, I think the proposal is missing some clear rules for when you can’t use type hints. Sure, when the whole thing throws a fatal error when you run it – but also if your code doesn’t yet depend on PHP 7 and you try using primitive type hints (or one of the newer ones that don’t even work with 7.0 – think contrib code that should still run on D 8.6 for half a year), or (as discussed in the other issue) in case of interfaces or of classes that might be sub-classed by other modules.
I think this needs to be made pretty clear, and specified very carefully, to avoid someone accidentally wreaking havoc in some other contrib module.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Issue summary: View changes

Good point about object types, clarified that a bit.

Regarding the PHP 7 requirement: Not sure yet how we could phrase that in a good way.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Related issues: +#2567861: Add test for enabling modules with incompatible PHP version dependency

Not sure yet how we could phrase that in a good way.

How about:

You must either:

  • require PHP 7 in your composer.json
  • require PHP 7 in your *.info.yml

For the second, see #2567861: Add test for enabling modules with incompatible PHP version dependency.

super_romeo’s picture

super_romeo commented

With 

declare(strict_types=1);

and 

(..., string $message)

we should always do typecast for t() result:

make_cat_speak(..., (string) t('Hi!'));

Not so cute :(

pfrenssen’s picture

pfrenssen
Location Sofia
commented

I think this example shows in fact how helpful this proposal is. This highlights that the make_cat_speak() function only works with static strings and does not support multilingual. It should be refactored to accept TranslatableMarkup objects.

super_romeo’s picture

super_romeo commented

OK.
And that about functions, that now accepts TranslatableMarkup/string parameters?

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented

And that about functions, that now accepts TranslatableMarkup/string parameters?

That’s where the “where possible” part of the proposal comes in. Not really much we can do for those, but that shouldn’t stop us from improving as much code as possible.

super_romeo’s picture

super_romeo commented

@drunken monkey, I'm in.

catch’s picture

catch
he/him
Primary language English
commented

+1 to this one - it's much easier to add these as we go along than it will be to change things later (which we already need to do for the existing code base eventually).

marios anagnostopoulos’s picture

marios anagnostopoulos commented

+1 for this.

Additionally, since drupal has PHP 7.4 and 8 as recommended versions, would it be correct to suggest Typed class variables in the coding standards?

Meaning no more

  /**
   * The language manager.
   *
   * @var \Drupal\Core\Language\LanguageManagerInterface
   */
  protected $languageManager;

Instead use 

/**
 * The language manager (Or any other more useful comment).
 */
 protected LanguageManagerInterface $languageManager;

But then again this would throw an error on 7.3 and below, so It would probably not be a good idea for core and contrib code, unless 7.3 and below stop having support. (Not sure if there are plans or not, but I guess this won't happen any time soon right?)

aaronmchale’s picture

aaronmchale
Primary language English
Location Edinburgh, Scotland
commented

Re #13: I would love to see that if we can do it, I think this might be the kind of thing that has to happen for 10.x, since we'll only support PHP 8+ there.

andyf’s picture

andyf commented

This would be lovely imho! My 2 pennies on the wording of one bit: on initial read the following two sentences seemed to me a little contradictory. It seems like the first is saying use an interface if there is one, but a class otherwise; while the last is saying it must be an interface.

DO NOT use a class as the type in type hinting (if possible). If specifying an object type, always specify an Interface.

Also, there's a distinction I don't think this makes clear. Sometimes when writing custom code that deals with a class defined elsewhere, I feel there ought to be an interface but isn't, and in that case I don't typehint at all, and just rely on a phpdoc @var or @return. (Feeling that typehinting the class itself would prevent being able to pass in/out a decorator for example.) However in other cases there are plain old data classes that I don't expect to have interfaces (eg. \Drupal\commerce_price\Price in Commerce) and using the class as the typehint seems fine. And in other cases I use the class just because everyone else does, and I pretend I understand why, but secretly I wonder why there isn't an interface (eg. \Drupal\Core\Database\Connection).

Finally, I'm not sure anyone's mentioned nullable types and void return types. They were only introduced in 7.1, but with 2 Nov approaching, I guess that's less of an issue?

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Active » Fixed

So this was actually already adopted as policy after the last version of Drupal that supported PHP 5 went out of security coverage:
https://www.drupal.org/docs/develop/standards/coding-standards#s-paramet...

At the time, I recall that we discussed that the entire page at https://www.drupal.org/docs/develop/standards/object-oriented-code should go away and be deduplicated/merged with the rest of our coding standards. Ten years after adopting PSR-0 and de-proceduralizing the majority of our codebase, "object-oriented code" is no longer a special concept, and obviously having docs in two places causes problems. :)

We furthermore have started to introduce nullable types in new APIs as the risk of disrupting backports to PHP 8.9 dropped to virtually none (and actually none now that it is EOL).

Regarding class vs. interface type hinting, the standard has already been that you typehint to the broadest interface (highest ancestor) that is possible for your API, or a class if the class is the public API (e.g. for value objects/final classes etc. that do not have multiple implementations).

So I don't think there's anything more to do here? The only thing we need to do is define a deprecation process for making the types of existing code stricter, by triggering deprecation warnings when a non-matching type is passed in beginning in a minor, and then adding the strict typehints for those deprecations in the next major.

Thanks everyone!

xjm’s picture

xjm
she/her
Primary language English
commented

The remainder of the work (like defining how we add typehints to old code in a BC way compatible with the continuous upgrade path) can be addressed in #3154762: [policy, no patch] All new Drupal 10 code should use appropriate type hints wherever possible and its followups. (They were sort of duplicates anyway). :) Thanks!

Status: Fixed » Closed (fixed)

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