Add return types to EntityDefinitionTestTrait

Created on 12 June 2025, 2 days ago

Problem/Motivation

Child issue of πŸ“Œ Determine if there are other TestTraits we can add return types to. Active . Adding return types to 30 functions removes 1848 lines from the baseline.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

πŸ“Œ Task
Status

Active

Version

11.0 πŸ”₯

Component

other

Created by

πŸ‡¦πŸ‡ΊAustralia mstrelan

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

Merge Requests

Comments & Activities

  • Issue created by @mstrelan
  • Merge request !12363Resolve #3529714 "Add return types" β†’ (Open) created by mstrelan
  • Pipeline finished with Success
    2 days ago
    Total: 415s
    #520265
  • πŸ‡¦πŸ‡ΊAustralia acbramley

    Based on https://www.drupal.org/project/drupal/issues/3529510#comment-16141888 πŸ“Œ Determine if there are other TestTraits we can add return types to. Active

    However, I'm pretty sure if you override a method from a trait then the trait method effectively does not exist, so you can do whatever you want with the signature and PHP won't care. See https://3v4l.org/kW8DR

    All but 2 of these functions are adding void so those are all safe.

    The other 2 already had @return docs so they aren't technically required here to cleanup the baseline, should we exclude them to make this easier to get in?

  • πŸ‡¦πŸ‡ΊAustralia mstrelan

    Good point, thanks for pointing that out. I've reverted those two.

  • πŸ‡¦πŸ‡ΊAustralia mstrelan
  • Pipeline finished with Failed
    2 days ago
    Total: 604s
    #520309
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    Seems pretty straight forward.

  • πŸ‡ΊπŸ‡ΈUnited States xjm

    I'd expect an RTBC for this to describe how precisely the many many methods included in this MR were reviewed to ensure that it was actually intentional that they had no return value. :)

  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    28 methods none had @return

    Trait itself getUpdatedEntityTypeDefinition and getUpdatedFieldStorageDefinitions are the only methods that have a return and those were untouched.

  • πŸ‡ΊπŸ‡ΈUnited States xjm

    @smustgrave, I was implying that you should actually read the methods (as I stated explicitly on the new sub-meta). :) In general, for any issues that add typehinting, you should actually read the code (at least briefly) rather than relying solely on the docs or static analysis, unless it's impossible in context for the typehint to be anything other than what was added (as it was for actual test runner methods in test classes). As per #3529510-11: [meta] Add return types to test traits because there is no BC break! β†’ :

    We simply need to open the methods and verify that they don't return anything under any circumstance (and moreover weren't supposed to, which static analysis can't tell us).

    Since we need to review the whole methods to verify that they are, indeed, intended to be void, the line length of the methods is the factor we want to look at

    I will be reading (or at least scanning) every method in the trait to review this for commit.

  • πŸ‡ΊπŸ‡ΈUnited States xjm

    I read over the full trait. The majority of methods on the trait simply set something-or-other in state; the others perform various operations like entity updates or changes to the entity storage. None had any conceivable code path that could ever have contained a return value.

    Saving credits. I'm granting credit for #8 as a best effort, although I would prefer to see a little more detail (and actual review of the code, not just the docs) in future issues for this meta in order to receive credit. Thanks!

    • xjm β†’ committed 0533816e on 11.x
      Issue #3529714 by mstrelan, xjm, smustgrave, acbramley: Add return types...
Production build 0.71.5 2024