Add array shape typing to definitions in MenuLinkManager

Created on 11 May 2025, about 2 months ago

Problem/Motivation

We can improve the typing of the return values of methods of MenuLinkManagerInterface so PHPStan on higher levels doesn't complain so much. This is primarily useful for consumers of MenuLinkManager, as core is currently at a lower level with a large baseline.

Note that the menu definitions returned by MenuLinkInterface and descendants (MenuLinkBase, MenuLinkDefault, etc), and MenuTreeStorage/MenuTreeStorageInterface are distinctly different, so they do not share the same type.

MenuLink definitions can have arbitrary keys, and MenuTreeStorage also has level (P*) columns.

I think MenuLinkItemInterface etc will need to use a generic, anyway.

Steps to reproduce

Make calls to MenuLinkManagerInterface methods in a private project with a high PHPSTan level.

Run \PHPStan\dumpType() on the result of the methods.

See the results do not have any keys or typing.

Proposed resolution

Introduce two PHPStan types, which have array shapes of menu link definitions.

One is a fully hydrated definition, the other is a partial, which includes the same typing, except all keys are optional.

Remaining tasks

Improve docs.

User interface changes

Nil.

Introduced terminology

Nil.

API changes

PHPDocs

Data model changes

Nil.

Release notes snippet

Nil.

📌 Task
Status

Active

Version

11.0 🔥

Component

menu system

Created by

🇦🇺Australia dpi Perth, Australia

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

Merge Requests

Comments & Activities

  • Issue created by @dpi
  • 🇦🇺Australia dpi Perth, Australia
  • 🇦🇺Australia dpi Perth, Australia
  • Pipeline finished with Success
    about 2 months ago
    Total: 509s
    #494201
  • 🇦🇺Australia dpi Perth, Australia
  • 🇧🇪Belgium borisson_ Mechelen, 🇧🇪

    I'm unsure about this, while we do allow phpstan types to be added in code since a recent coding standards update, this would be the first time we'd be adding a type like this as far as I know.

    I'm not sure it was the intention of the coding standards committee to allow us to introduce a typing as complex as this one.

    How do we make sure we keep the type compatible with the actual array structure, can phpstan help us with this or is that something we'd need to define in a test?
    Should we not bother?

    I do see the value in doing this though, it makes IDE's so much happier.

  • 🇦🇺Australia dpi Perth, Australia

    How do we make sure we keep the type compatible with the actual array structure, can phpstan help us with this or is that something we'd need to define in a test?
    Should we not bother?

    When our internal usage changes, either regular runtime or tests, PHPStan will display errors.

    For example if we add a new array key to the structure, and a test makes use of that new key, PHPStan will complain the key does not exist.

    The same applies to removals, type changes, restructures, etc.

  • 🇮🇹Italy mondrake 🇮🇹

    This would be very very nice, but aside from @borisson_ comment above, I wonder whether we are ready for it.

    If I am not mistaken, https://phpstan.org/user-guide/rule-levels, the actual checking of the array shapes' contents will start from PHPStan level 5 above; we are currently struggling with level 1...

    So we may be introducing non-validated typing that could become a barrier rather that a facilitation to move to upper levels. Happy to be mistaken though.

  • 🇧🇪Belgium borisson_ Mechelen, 🇧🇪

    If I am not mistaken, https://phpstan.org/user-guide/rule-levels, the actual checking of the array shapes' contents will start from PHPStan level 5 above; we are currently struggling with level 1...

    This is already noted in the Issue Summary as well. I agree - I don't think we can start doing this without us being on level 5 as well, because the arguments in #7 need level 5 to be valid.

  • 🇦🇺Australia dpi Perth, Australia

    the actual checking of the array shapes' contents will start from PHPStan level 5 above; we are currently struggling with level 1...

    - I don't think we can start doing this without us being on level 5 as well,

    Level 2, not 5, by the looks: https://phpstan.org/r/63bdb4f4-ca81-4423-8828-9da62ef4a5c9

    According to that try page, perhaps it is enforcable with just enabling reportPossiblyNonexistentGeneralArrayOffset, and we could baseline on that.

    Stan and typing doesnt start and end with core. This change benefits, and is driven by, contrib and custom code.

    As a project using PHPStan Lvl 9 + Strict I can immediately benefit from this.

    Whether we enforce this in core now or later shouldn't matter too much, if non-core is consuming it, then it can also be responsible for reporting back to core. Just like we have to right now with the multitude of unenforced defective PHPDocs. When it comes time to core enforcing it, the amount of code is drastically reduced to simple bulk fixups, just like we would with existing PHPStan bulk changes.

Production build 0.71.5 2024