Problem/Motivation

With the introduction of PHP 7.4 the language now supports indicating what type a class property is (in older versions this was limited to PHP primitive types). In the current Drupal Coding Standards a @var annotation is required for class properties. However, since this information can now be added in code (when targetting at least PHP 7.4), those comments are often duplicated information that risk becoming out of sync.

Steps to reproduce

Proposed resolution

Adjust the coding standards to allow not adding a @var comment to class properties in case those properties have a type defined using PHP's typing system.

#3123282: Do not require @var tag if a property has typehint provides a patch for coder that demonstrates this behaviour.

The following code would now be valid.

class Foo {
  
  /**
    * Where one can order a soda.
    */
  protected Bar $baz;

  /**
    * Some complex type that probably needs cleaning up in a future.
    * 
    * This type can not be represented in the PHP typesystem yet.
    *
    * @var int|string|NULL
    */
   public $someVar;

}

Documentation changes

1. @var: Class property data types

Current text
class FooBar {

  /**
   * The database connection object for this statement's DatabaseConnection.
   *
   * @var \Drupal\Core\Database\Connection
   */
  public $dbh;
}

Syntax notes: The @var tag is followed by a space and then a data type specification.

Proposed text
class FooBar {

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

Syntax notes: The @var tag is followed by a space and then a data type specification.

class FooBar {

  use \Drupal\Core\Entity\EntityTypeManagerInterface;

  /**
   * The entity type manager.
   */
  protected EntityTypeManagerInterface $entityTypeManager;
}

Typed class properties may omit the @var declaration. It is recommended to use typed properties whenever possible.

Remaining tasks

Comments

Kingdutch created an issue. See original summary.

chi’s picture

chi commented

This type can not be represented in the PHP typesystem yet.

It can be presented using union type hint in PHP 8.

drunken monkey’s picture

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

+1

pfrenssen’s picture

pfrenssen
Location Sofia
commented

+1

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Status: Active » Reviewed & tested by the community

+1. I think it makes sense.

The Coder PR is ready for review https://github.com/pfrenssen/coder/pull/164

pfrenssen’s picture

pfrenssen
Location Sofia
commented
Issue tags: +Needs announcement for final discussion
romain.sickenberg’s picture

romain.sickenberg
Primary language French
Location Lausanne
commented

+1!

mondrake’s picture

mondrake
Location 🇮🇹
commented

+1

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

FWIW another +1 from me, we've already adopted this in our client projects

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented

Another +1; we now need to go trough announcement and final discussions.

donquixote’s picture

donquixote commented

+1, yes!

ghost of drupal past’s picture

ghost of drupal past
he/him
commented

Good first step but needs several followups

  1. Allow omitting doxygen altogether. It's just not necessary to add any doxygen to protected LockbackendInterface $lockBackend. Optionally yes but most of the time the type tells the story
  2. Extend to @param and @return and entire method doxygen. Once again, these might be completely unnecessary. protected function getLockbackend(): LockbackendInterface , gee, I have no idea what this method could possibly be about, let me try to guess.
quietone’s picture

quietone commented
Issue summary: View changes

Add the existing documentation to the Issue Summary.

It will help if someone would complete the proposed text section of the Issue Summary.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Issue summary: View changes

Took a shot at a proposed text

ghost of drupal past’s picture

ghost of drupal past
he/him
commented

Filed #3376518: Allow omitting @var, @param and @return tags or their descriptions if the variable name and type is enough for a followup.

quietone’s picture

quietone commented
Issue summary: View changes

@larowlan, thanks.
I like examples, so I expanded on suggestion.

quietone credited alexpott.

quietone credited catch.

quietone credited longwave.

quietone credited Spokje.

quietone’s picture

quietone commented

Closed #3291517: [policy, no patch] Use type declarations for new class properties as a duplicate. I am adding credit.

quietone’s picture

quietone commented
Issue tags: -Needs announcement for final discussion +final discussion
tyler36’s picture

tyler36 commented

+1 Feels like we are embracing PHP conventions.

gngn’s picture

gngn commented

+1

bbrala’s picture

bbrala
Location Netherlands
commented

This has been standard practice in our client code for ages. +1

dww’s picture

dww
we/he/they
commented

+1, yes please!

dww’s picture

dww
we/he/they
commented
Issue summary: View changes

Remove duplicate <code> from proposed res

dww’s picture

dww
we/he/they
commented
  1. s/type hint/strict type definition/?
  2. Should the new text say that strict types are preferred wherever possible?
mondrake’s picture

mondrake
Location 🇮🇹
commented

IMHO the examples should be more generic. The $dbh property is long gone from the Database API and regadless of that it was a very special case being a PDO related property.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

🙏 Yes, please! Far less boilerplate 👍

urvashi_vora’s picture

urvashi_vora commented

+1

pfrenssen’s picture

pfrenssen
Location Sofia
commented
Issue summary: View changes

Updated the wording taking into account the suggestions from #28 and #29. Switched from a public property to a protected one. Used the EntityTypeManagerInterface since this is very recognizable, and uses the interface as a type which is the best practice.

catch’s picture

catch
he/him
Primary language English
commented

I agree with this as it stands, but it seems worth noting that #3278431: [May 2026] Use PHP 8 constructor property promotion for existing code means that any property promoted via a contructor won't be defined separately at all, so the entire statement goes in that case. That makes me wonder how many strictly typed properties will be left to document once that issue lands.

quietone’s picture

quietone commented
Status: Reviewed & tested by the community » Needs work

I think we missed a step, according the current process this needs to have a working patch to test the change.

All changes that can be tested with coder/phpcs. A working patch is to be provided with the issue. Committers will test the phpcs rule as part of the approval process.

And what happens to the existing sniff, currently excluded, <exclude name="Drupal.Commenting.VariableComment.Missing"/>

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Reviewed & tested by the community

If we follow the new proposal at #3365085: Update the coding standards process on the project page then based on the current situation, we should have checked the existing sniff (which you just have), but we're not blocked on a new one.

Since <exclude name="Drupal.Commenting.VariableComment.Missing"/> is already disabled and therefore changing the rule won't break core's existing enabled ruleset.

That would get us to:

If there is no existing phpcs rule for the change, then optionally a patch can be provided on the coding standards issue to implement the rule. Once the coding standards issue is fixed, an issue can be opened either against phpcs or coder to add support for the rule. This is to prevent issues being opened against coder/phpcs for rules which core might not adopt.

in the new process.

i.e. we should open an issue against coder asking for either a new sniff, or a modification to <exclude name="Drupal.Commenting.VariableComment.Missing"/>, as part of the process of marking this issue fixed.

Based on that, moving back to RTBC.

If we want to follow the documented process, then I think we should just update the new process to what we've got proposed rather than trying to follow the old one.

quietone’s picture

quietone commented
Issue tags: -final discussion

I have updated the documentation.

bbrala’s picture

bbrala
Location Netherlands
commented

Confirmed the changes in the documentation. This can be marked as fixed.

bbrala’s picture

bbrala
Location Netherlands
commented

Fixing credits:

Kingdutch: made issue, discussed in slack.
quietone: managing this the last few months, and driving discussion.
larowlan: heping out with issue management and discussion
chx: discussion, and followup work.
bbrala: discussions and issue management
pfrenssen: work on docs
dww: work on docs

bbrala’s picture

bbrala
Location Netherlands
commented
Status: Reviewed & tested by the community » Fixed

Marking as fixed. Lovely to see a codingstandards issue fixed. <3

Thanks everyone.

Status: Fixed » Closed (fixed)

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

penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾 Seville 💃 Andalusia, UTC+2 🇪🇺
commented

Did we ever create a follow-up for converting existing code? It's hard to enforce this policy when consistency matters.

bbrala’s picture

bbrala
Location Netherlands
commented

Probably not, just make one?

bbrala’s picture

bbrala
Location Netherlands
commented

Created a child: #3522636: Remove @var for strictly typed class properties