[policy, no patch] All new Drupal 10 code should use appropriate type hints wherever possible

I'm plus one on this, because its going to add more future work if we don't do it sooner.

+1, I would also suggest to declare nullability of an argument prefixing the type name with a question mark like e.g.

public function lastInsertId(?string $name = NULL): string {
...
}

This was introduced in PHP 7.1 so no problem for D9.

I'm on board with this -- my first thought was "This will make backports a pain", but of course we don't backport new code and something calling new code is going to diverge anyway.

What I am foggy on but want to solve is how to add typehints (of all flavors) in a BC way, strategically across core. We can make this decision without that, but the inconsistency will also send us to a place where the DX of what's typehinted and what's not seems random. That's a problem we need to solve, and it'd be nice if we could come up with a way that wasn't "create a duplicate of every interface and class in core" or whatever.

See discussion on #3050720: [Meta] Implement strict typing in existing code. Not sure about return type hints, but for scalar typehints on interfaces there is a properly forward-compatible approach - just requires a fair bit of shuffling.

Bumping this as #256287: Give roles a description value proposes adding two new methods to RoleInterface and I think they should be typed, because why not?

Is there a good reason *not* to adopt this policy? Existing code has its own challenges but this is fairly straightforward for new code, as far as I can see.

Bumping this one - I think we should get it documented, and open a follow-up to figure out existing code. But the quicker we do this for new code the less we'll have to change later (although only a fraction of the total).

PHP allows adding return typehints to child implementations even if the parent doesn't have one, so D9 contrib code can add the return typehints to their implementations (edit: so long as they are compatible with the return values and docs already present in core) and be forward-compatible with D10.

For the past year or so I've requested return typehints on basically everything new, including child implementations.

A category of thing that gets overlooked a lot is test methods, which could always have a void return typehint (a pattern we're inheriting from phpunit). Those aren't really providing new information, but OTOH they would sure provide a lot of visibility to remind people to add typehints.

Added initial docs here:
https://www.drupal.org/docs/develop/standards/coding-standards#s-paramet...

It could probably do with some examples.

We should also draft and publish a change record that links that coding standards section.

Related: The added docs partly duplicate part of https://www.drupal.org/docs/develop/standards/object-oriented-code#hinting. That page no longer makes sense as a separate page since "object-oriented code" is no longer a useful distinction from "pretty much all our code". Much of the documentation is furthermore obsolete. We should add the relevant parts to the main coding standards documentation page, and delete the rest along with the separate page.

public function checkAccess(string $permission) : \Drupal\Core\Access\AccessResult

That needs a coding standard about whether or not to put space in front of : in return type hint.

Agreed.

Though the example so far is wrong anyway -- we'd never have a fully-qualified classname, it would be imported.

The options are:

    public function checkAccess(string $permission) : AccessResult // space each side
    public function checkAccess(string $permission) :AccessResult // space before
    public function checkAccess(string $permission): AccessResult  // space after
    public function checkAccess(string $permission):AccessResult  // no space at all

The style I've seen used on projects I've worked on is space after, so:

    public function checkAccess(string $permission): AccessResult  // space after

though it does mean that where there are no parameters, I always see it like a sideways Homer Simpson:

    public function myFunction(): string

For the record Drupal Core and Symfony currently use "space after" in ~99% cases. On the other hand Laminas mainly follows "space each side" rule.

With that said, "space after" is the obvious choice.

Updating IS, mostly trying to identity the remaining steps. Updated the example based on feedback in #16.

I've just noticed this problem with the example in the IS:

> public function checkAccess(string $permission): AccessResult {

That's not a scalar typehint, surely? By 'scalar' I assume what is meant is primitive types like int, string, bool.

> use scalar typehints and return typehints
I think that meant scalar typehints for parameters and non-scalar typehints for return values. I guess it complies with the PHP versions that Drupal 9 is currently supported.

Anyway, I think there is no point to distinguish between scalar and non-scalar typehints nowadays.
Furhtermore, since PHP 8, it does not make sense to apply different policies for parameters, class properties and return types.

Given that Drupal 10 is close it might be better move the issue to 10.x.
Proposed issue title: All new Drupal 10 code should use appropriate typehints wherever possible.

Yes let's do that.

The only thing with adding them to Drupal 10 is if we're going to backport the new code to 9.4.x/9.3.x we still have to be careful about PHP >= 7.3, but that won't be the case for the 10.1.x branch.

Do we want to include constructor property promotion here or should that be its own issue?

> Do we want to include constructor property promotion here or should that be its own issue?
I think it needs its own issue.

The current issue is two years old and so far no one objected. So I suppose having change record is the last step needed to move this to RTBC.

One thing that is not covered yet is what to do with PHPDoc annotations. The has become obsolete.

In the example below the property annotation is absolutely useless. Actually the whole comment makes no sense.

/**
 * The entity type manager.
 *
 * @var \Drupal\Core\Entity\EntityTypeManagerInterface
 */
protected EntityTypeManagerInterface $entityTypeManager;

@chi that's got a lot to do with constructor property promotion, the entire code block can go once we do that.

Once Drupal 9 is EOL and we don't have to think about backports I can foresee an issue where we convert all constructors in core to use property promotion, hopefully with the aid of a Rector rule.

Well, that's actually not only about class properties. Method parameters and return types also have lots of redundant annotations.

I've recently started ommiting all doc comments for methods, properties and constants. That accelerated develpment ~20%, I guess.

Documentation typehints can add information that PHP typehints do not yet support, especially for array structures. It's very useful to document string[][], mixed[], etc.

Documentation typehints can add information that PHP typehints do not yet support

Right, for those cases we can keep the documentation which by the way can be written using PHPStan or #[ArrayShape] notation.

Is all that is left to do here to write the change record? I think in practice we are already doing this in new code, though I recently proposed the same for class properties in #3291517: [policy, no patch] Use type declarations for new class properties.

Totally agree that we should remove redundant doxygen for methods, properties and constants. It is quicker to read and write.

Anyone point out an example change record that doesn't have code?

#29 and #30 see #3309010: Support PHPDoc Types in @param @var @return annotations

Totally agree on the proposed solution.
New methods and functions added in Drupal 9 should use scalar typehints and return typehints.

"space after" looks more for the obvious choice

Do we really need a change record for an MR that is policy and has no code change at all? This has been sitting near the finish line for a year.

In the list of reasons for adding a change record I don't see anything that covers this. The closest is "For Coding standard issues when the rule requires significant code changes" but that would apply to existing code, not new code. Based on that I am removing the tag.

We have been doing this for some time and the Coding Standard docs have changed. Unless I am missing something there is nothing more to do in this issue.

Given this has been sitting here for two years and in practice we have been doing it already, a change record seems pointless now.

.

Changing to latest version when this was closed.