Introduce a @return-type-will-be-added annotation to indicate return type addition in next major; enforce with a PHPStan rule

Created on 8 November 2024, 2 months ago

Problem/Motivation

In 📌 Ensure getWidth()/getHeigth always return ?int Needs review we are using the issue to try and introduce a process to signal a native return type will be added to interface methods in next major release (and in 📌 Add return typehints for getWidth()/getHeigth in interfaces Postponed we have the follow up to actually implement the change in the next major). The concept is that the intended type is indicated to extensions, and given return type covariance allows them to implement the type in advance of the change on interface/abstract/base class, we prod contrib/custom to do the change to be ready when next major (implementing the type) is out.

More or less the same intent in 📌 Enable existing interfaces to add return type hints with a deprecation message for implementors Active .

There are discussion to support this through Symfony's DebugClassLoader - see [ErrorHandler] DebugClassloader: introduce an annotation to signal definite intent to add/change the return typehint of a method and a PR [ErrorHandler] Add support for @return-type-will-change

This issue explores an alternative approach, introducing a PHPStan custom rule to do the checking on static analysis rather than test time.

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

📌 Task
Status

Active

Version

11.0 🔥

Component

base system

Created by

🇮🇹Italy mondrake 🇮🇹

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @mondrake
  • Merge request !10111Closes #3486376 → (Open) created by mondrake
  • Pipeline finished with Failed
    2 months ago
    Total: 196s
    #333504
  • Pipeline finished with Failed
    2 months ago
    Total: 243s
    #333511
  • Pipeline finished with Failed
    2 months ago
    Total: 147s
    #333514
  • 🇮🇹Italy mondrake 🇮🇹

    Needs tests for the rule and some cleanup, but I am opening it for review of the concept.

    This shows how adding the @return-type-will-be-added to some methods of ImageInterface, PHPStan identifies that the implementing methods on the concrete Image class need to get the return type.

  • 🇮🇹Italy mondrake 🇮🇹
  • Pipeline finished with Failed
    2 months ago
    Total: 158s
    #333837
  • Pipeline finished with Failed
    2 months ago
    Total: 180s
    #333846
  • 🇮🇹Italy mondrake 🇮🇹

    Reverted changes to code that were meant to showcase the rule. The example pipeline were the PHPStan output is visible is https://git.drupalcode.org/issue/drupal-3486376/-/pipelines/333846

    Added PHPUnit tests for the PHPSTan rule.

  • Pipeline finished with Failed
    2 months ago
    Total: 395s
    #333972
  • Pipeline finished with Success
    2 months ago
    Total: 666s
    #334002
  • Pipeline finished with Success
    2 months ago
    Total: 2373s
    #334010
  • Pipeline finished with Failed
    2 months ago
    Total: 540s
    #334523
  • Pipeline finished with Failed
    2 months ago
    Total: 160s
    #334564
  • Pipeline finished with Failed
    2 months ago
    Total: 266s
    #334567
  • Pipeline finished with Success
    2 months ago
    Total: 1581s
    #334570
  • 🇫🇷France nod_ Lille

    this makes the bot crash, i don't have 10G or ram on it

  • 🇺🇸United States smustgrave

    Wonder if this counts as an API change?

    Or least warrants a CR?

    Only moving to NR for the pipeline revert as not sure it's in scope.

    If can rebase too since it's kinda older (sorry took so long to look at)

    If you are another contributor eager to jump in, please allow the original poster @mondrake at least 48 hours to respond to feedback first, so they have the opportunity to finish what they started!

  • Pipeline finished with Failed
    26 days ago
    Total: 193s
    #379466
  • 🇮🇹Italy mondrake 🇮🇹

    It certainly needs a CR to explain how to use the annotation, but let's get more reviews and consensus before writing one and letting it go stale because of later changes of direction.

    The path addition in CI is needed to get the rule tested (HEAD is currently checking only changes to file in the base path of Drupal's PHPStan-related stuff).

    The Symfony classloader proposed changes were closed without commit; this remains the only approach currently workable to address this issue.

  • Pipeline finished with Canceled
    26 days ago
    Total: 130s
    #379482
  • Pipeline finished with Success
    26 days ago
    Total: 1537s
    #379484
  • 🇮🇹Italy mondrake 🇮🇹

    BTW I am not even sure this is for core or rather for mglaman/phpstan-drupal.

  • Issue was unassigned.
  • Status changed to Needs review 8 days ago
  • 🇺🇸United States smustgrave

    Probably phpstan-drupal right?

Production build 0.71.5 2024