Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.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.
Update calls and usages where definitions are passed around, making the calls more strict. Where getDefinition is used to determine existence, switch to hasDefinition instead.
Remaining tasks
Improve docs.
User interface changes
Nil.
Introduced terminology
Nil.
API changes
PHPDocs
Data model changes
Nil.
Release notes snippet
Nil.
Issue fork drupal-3523800
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #2
dpiComment #4
dpiComment #5
dpiComment #6
borisson_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.
Comment #7
dpiWhen 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.
Comment #8
mondrakeThis 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 later a barrier rather than a facilitation to move to upper levels. Happy to be mistaken though.
Comment #9
borisson_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.
Comment #10
dpiLevel 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.
Comment #11
smustgrave commentedTrying to follow but if this can't be checked in core till a different level then shouldn't it be postponed. Not sure I see many issues land that are only for contrib and not useable by core.
Comment #12
borisson_That's true, there's usually the rule that there needs to be at least one usecase in core. But I think that's mostly about code-implementations.
This will help for core development as well, since it will help everyone working with this that is using an IDE with code completion.
In hopes not to derail this issue too much, I wonder if it's a good plan to start with one random thing that returns an array as we're doing here or if we should form a plan first.
Comment #13
smustgrave commentedNot going to pretend I can speak on this one. But seems like from the comments it would be better to first formulate a plan to start implementing these kind of changes no?
Comment #14
borisson_I talked about this with Matt Glaman at drupalcon last week. I'm hoping he can find a good solution for this, while a plan would be nice, I think we can start small and start adding it in places where we can add value.
I just really don't want to add this without it being validated, but it sounded like Matt had a good solution for that.
Comment #15
smustgrave commentedSo would this be the best status? Can change back if I'm wrong.
Comment #16
smustgrave commentedThanks for sparking that conversation too!