Problem/Motivation

PHP 7 introduces return type declarations. We should define a standard for using them. It would be interesting to follow the standard that is (or will be) adopted by the wider PHP community. Return types are not described in any of the existing standards like PSR-2, but it is part of PSR-12 which currently is in draft version (ref. https://github.com/php-fig/fig-standards/blob/master/accepted/PSR-12-ext...).

Here are the relevant sections from the standard:

When you have a return type declaration present, there MUST be one space after the colon followed by the type declaration. The colon and declaration MUST be on the same line as the argument list closing parenthesis with no spaces between the two characters.

<?php

declare(strict_types=1);

namespace Vendor\Package;

class ReturnTypeVariations
{
    public function functionName(int $arg1, $arg2): string
    {
        return 'foo';
    }

    public function anotherFunction(
        string $foo,
        string $bar,
        int $baz
    ): string {
        return 'foo';
    }
}

In nullable type declarations, there should be no space between the question mark and the type.

<?php

declare(strict_types=1);

namespace Vendor\Package;

class ReturnTypeVariations
{
    public function functionName(?string $arg1, ?int &$arg2): ?string
    {
        return 'foo';
    }
}

We can start with this as a basis, rewriting it a bit in the "more human friendly" writing style that we typically use, and rework the examples to be more Drupally.

The related sniff is SlevomatCodingStandard.TypeHints.ReturnTypeHintSpacing.

Benefits

If we adopted this change, the Drupal Project would benefit by ...

Three supporters required

  1. https://www.drupal.org/u/akulsaxena (2024-11-27)
  2. https://www.drupal.org/u/mstrelan (2025-02-03)
  3. https://www.drupal.org/u/{userid} (yyyy-mm-dd they added support)

Proposed changes

Provide all proposed changes to the Drupal Coding standards. Give a link to each section that will be changed, and show the current text and proposed text as in the following layout:

1. https://www.drupal.org/docs/develop/standards/php/php-coding-standards#t...

Current text

Beginning with Drupal 9, parameter and return type hints should be used wherever possible. Example function definition using parameter and return type hints:

public function myMethod(MyClass $myClass, string $id): array {
  // Method code here.
}
Proposed text

Beginning with Drupal 9, parameter and return type hints should be used wherever possible. There should be no space between the closing parentheses and the colon, and one space after the colon before the return type. 

public function myMethod(MyClass $myClass, string $id): array {
  // Method code here.
}

In nullable type declarations, there should be no space between the question mark and the type.

public function myMethod(?MyClass $myClass, ?string $id): ?array {
  // Method code here.
}

2. Repeat the above for each page or sub-page that needs to be changed.

Remaining tasks

  1. Create this issue in the Coding Standards queue, using the defined template
  2. Add supporters
  3. Create a Change Record
  4. Review by the Coding Standards Committee
  5. Coding Standards Committee takes action as required
  6. Discussed by the Core Committer Committee, if it impacts Drupal Core
  7. Final review by Coding Standards Committee
  8. Documentation updates
    1. Edit all pages
    2. Publish change record
    3. Remove 'Needs documentation edits' tag
  9. If applicable, create follow-up issues for PHPCS rules/sniffs changes

For a full explanation of these steps see the Coding Standards project page

Comments

pfrenssen created an issue. See original summary.

drunken monkey’s picture

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

+1

At the latest, we should have a standard for this once #2842431: [policy] Remove PHP 5.5, 5.6 support in Drupal 8.7 becomes applicable.
PSR-12 looks pretty good there in this case, so yes, why not go with it? We'll just also need to define that there should (I'd say) be a single space between the type and the opening brace of the function body (PSR-12 doesnt need that since PSR-2 says the brace goes on the next line):

  public function foo(): ?string {
    …

The more interesting question, in my opinion, would be whether we also want to encourage, or even "force", people to use return type declarations now, if possible. I.e., not only "how should it look" but also "it really should [or even "must"] be there, if possible". ("If possible" because some functions/methods can, of course, return different types – it's not really ideal for clean code, imo, but that's how it is.)

I'd be +1 for encouraging people to include them, too.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented

+1

What about the declare(strict_types=1); line? Shouldn't we add some rule about it?

sardara’s picture

sardara commented

+1 for the no space before the colon and +1 for the space before the opening brace of the function body.

sophie.sk’s picture

sophie.sk
she/her
Primary language English
commented

+1 here! Now that #2842431: [policy] Remove PHP 5.5, 5.6 support in Drupal 8.7 is marked as fixed, it seems sensible to start thinking about this (and other PHP 7 conventions). If devs are already developing on and for PHP 7+, then it makes sense to define the coding standards for it.

drunken monkey’s picture

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

What about the declare(strict_types=1); line? Shouldn't we add some rule about it?

Regarding the format, I'd also say PSR-12. And I'd allow it, but not encourage it. In PHP, where “numbers” often come as numeric strings (especially from the DB), I personally found strict types to be more of a hassle than a help – even though I'm usually all for stricter standards. But if someone wants to use them, why not? Most of the effect will be restricted to that file anyways, and they should just take care that they can still handle things like numeric strings coming in (e.g., even when they aren't a direct parameter, and so can't be auto-cast).

pfrenssen’s picture

pfrenssen
Location Sofia
commented

I think the declaration of strict types should just follow our existing standard, so people should write it like this:

declare(strict_types = 1);

No space before the opening parentheses, and a space on each side of the assignment operator. Same as with the return types, we cannot encourage this before Drupal itself drops support for PHP5, but we can define a coding standard.

effulgentsia’s picture

effulgentsia commented
Title: Define a standard for PHP7 return types » Adopt the PSR-12 standard for PHP7 return types once Drupal 8 drops PHP 5 support
Issue tags: +needs announcement for final discussion

+1 from me as well. Tagging for an announcement and retitling for clarity.

tizzo’s picture

tizzo commented
Issue tags: -needs announcement for final discussion +final discussion
daffie’s picture

daffie commented

+1 for me.

Does implementing this cause a BC break. If so, how are we going to implement it then.

drunken monkey’s picture

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

Does implementing this cause a BC break. If so, how are we going to implement it then.

This issue is just about how to format type hints when you add them. It’s not about actually adding them whereever possible.
Of course, if you’d then go and add a return type declaration to an interface method, that would be a BC break. So, don’t do that. For now, I guess we’ll have to restrict use of return types to new code, or code not covered by our BC policy.

(With the current plans for Drupal 9, it sounds to me like applying return types throughout the code wouldn’t even be possible at that point? Interesting question how we could ever get this into existing interfaces and the like.)

slootjes’s picture

slootjes commented

+1 on this as I believe Drupal should fully switch to PSR for coding standards in general to attract more developers from other communities.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented

Interesting question how we could ever get this into existing interfaces and the like.)

I'm afraid that, to respect continuous upgrades, we'd have to provide alternative interfaces (or at least methods) and deprecate the old ones.

xjm’s picture

xjm
she/her
Primary language English
commented

Hmmm, I'm concerned about the disruption of adopting entire new PSRs in a minor. I'm also concerned that the PSR itself is still a draft.

xjm’s picture

xjm
she/her
Primary language English
commented

Looks like what @drunken monkey said is pretty helpful:

This issue is just about how to format type hints when you add them. It’s not about actually adding them whereever possible.

Of course, if you’d then go and add a return type declaration to an interface method, that would be a BC break. So, don’t do that. For now, I guess we’ll have to restrict use of return types to new code, or code not covered by our BC policy.

So I guess I'm not concerned about disruption so long as this is only about formatting if/when we ever add return types, not about actually adding return types. However, my second concern (about the PSR being a draft) remains.

kingdutch’s picture

kingdutch
he/him
Primary language English
commented

I've opened #3050720: [Meta] Implement strict typing in existing code to contain discussion about how we could tackle the BC compatibility issues for updating existing code. That way it can be kept separate from this discussion about adopting the standard for newly introduced code.

klausi’s picture

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

Created a coding standards proposal to update our coding standards to always use type hints as much as possible for all new code: #3060914: All new Drupal 8/9 code must use strict type hints where possible. Would love your +1 support and opinion there!

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
commented

@xjm - We would not be adopting the entire psr-12 standard, draft or not. PSR-12 is an all-encompassing coding standard which would replace PSR-2, and include things like PSR-2's 4 space indentations, and opening braces on the following line after a function/class definition, which would be largely disruptive and replace our entire coding standards.

As I read it, this proposal only seeks to add to our custom coding standard rules the small section of PSR-12 which relates to type hinting method return values. Our current coding standard doesn't address spacing/formatting around return type hints at all, since until recently we could not use them as they are php7.0+ specific.

+1 on gathering consensus and implementing a standard around this, though we need to work on the wording a bit.

One issue here is that the nullable return type hinting support shown in the IS requires PHP 7.1, and we support PHP 7.0, so we could not use nullable type hints until probably Drupal 9, which will require PHP 7.1 or more likely 7.2. Nevertheless, I wouldn't mind agreeing on the standard now, since the D9 branch will be opening at some point in the next 6 months anyway.

It begs the question, probably for xjm and the other RMs to answer: Do we want to use nullable type hints in the D9 branch when it opens, or do we hold off until D8 LTS is EOL in 2021? We have to require php 7.1 or 7.2 in D9 due to vendor requirements, but using 7.1 syntax in D9 will make it more difficult to backport bug fixes into D8 LTS.

ribeirobreno’s picture

ribeirobreno commented

+1 on adopting PSR for this. (And anything else, really. :] )

heddn’s picture

heddn
Primary language English
Location Nicaragua
commented

+1 on this. And I think is more than actionable at this time since PHP 8.9 only supports PHP 7.1+ and 9.x supports a minimum of PHP 7.2.

martin107’s picture

martin107 commented

+1 on this

We are now preparing to migrate to Symfony5, or at least allow for its inclusion

https://www.drupal.org/node/3055180

As a consequence of that

here is a non-obvious aspect to Symfony 5 modernisation.

The APIs are starting to enforce parameter and return types.

https://www.drupal.org/project/drupal/issues/3125234

We are in the process of modifying drupal9.1.x core code to use require parameter type and return types.

otherwise we will generate errors/"interface incompatibilites" with Symfony5 Interface classes.

Within that timescale this has now become necessary, not a "would like to have."

lolcode’s picture

lolcode commented

+1 on creating a standard. I am migrating a module to D9 and currently getting an error from the IDE coming from KernelTestBase.php and any of the subclasses I am writing:

Return type declaration must be compatible with TestCase->setUp() : void

As a contrib developer I would like to know whether to leave the return type off following Drupal core or whether to add it now for future compatibility.

pasqualle’s picture

pasqualle
Location 🇭🇺 Budapest
commented
Title: Adopt the PSR-12 standard for PHP7 return types once Drupal 8 drops PHP 5 support » Adopt the PSR-12 standard for PHP7 return types

issue title update

gekkie’s picture

gekkie commented

I'd also like to chime in: with the inclusion of Drupal into the wider PHP community (and supported by a more composer-esque view of the world) i'd also like to see a PSR-12 style adopted inside Drupal-core and the wider Drupal community. It's been a while since any updates in this regard. Is there anything one can do to help shepherd this along?

gekkie’s picture

gekkie commented

With the advent of PHP8.1 release, the ending of active support for PHP7.4 already passed is it now finally time to fully discuss the move to PSR-12 and have an up to date, clean and much wider adopted standard (outside of drupal) for _all_ PHP code? It might even solve various other issues in this queue that relate to various preferences...

moshe weitzman’s picture

moshe weitzman commented

FWIW, Drush has adopted PSR12 and its going well. Would love to see Drupal move in this direction.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Issue summary: View changes

Fixing link in the issue summary. Please don't take this as an endorsement of PSR12, it's very much not.

quietone’s picture

quietone commented
Issue summary: View changes
Issue tags: -final discussion

Updated issue summary with new template, the current text of PSR-12 and completed the existing Drupal text and proposed text.

Anonymous’s picture

Anonymous (not verified) commented
Issue summary: View changes

+1 on this.

quietone’s picture

quietone commented
Issue summary: View changes

Actually add the "?" to the proposed resolution

quietone credited mstrelan.

quietone’s picture

quietone commented
Category: Feature request » Task
Issue summary: View changes

Updated the issue summary to include #3477682: Explicitly require no space before colon for return types, which I closed as a duplicate, adding credit from that issue Change to a task because this is being done in practice.

mstrelan’s picture

mstrelan commented
Title: Adopt the PSR-12 standard for PHP7 return types » Adopt the PSR-12 standard for PHP return types
Issue summary: View changes

I think we can drop the number 7 from the title. No one is using PHP 7 any more, and PHP has evolved since return types were introduced. Adding my name as a supporter.