Problem/Motivation

The phpstan job has been failing on every pipeline for some time. It is
marked allow_failure, so pipelines still report success and the errors go
unnoticed. Two errors are reported, both in
src/Plugin/rest/resource/RestMenuItemsResource.php:

126 Storing entity storage as a class property is not recommended.
Call Drupal\Core\Entity\EntityTypeManagerInterface::getStorage()
at the call-site instead.
drupal.entityStoragePropertyAssignment

140 Unsafe usage of new static().
new.static

Neither is new. git blame puts them at 03a7b5cd (2023-09-29) and 12c9be0b
(2019-10-08). They are unrelated to any open issue, which is why this is
filed separately rather than being folded into #3615481.

Steps to reproduce

1. Check out 3.0.x.
2. Run the phpstan job, or locally:
vendor/bin/phpstan analyse --configuration= \
web/modules/contrib/rest_menu_items
3. Two errors are reported.

Proposed resolution

new.static (line 140)

PHPStan raises this because a subclass could declare a different
constructor. Making the class final is not an option: getMenuItems(),
getElementValue() and (as of #3615481) setup() are protected precisely so
the resource can be subclassed. The right fix is to add
@phpstan-consistent-constructor to the class doc comment, which tells
PHPStan the constructor signature is intended to stay the same in
subclasses.

drupal.entityStoragePropertyAssignment (line 126)

$menuLinkContentStorage is assigned in the constructor and used in exactly
one place, line 540:

$menu_link_content = $this->menuLinkContentStorage->load($id);

Replacing that with
$this->entityTypeManager->getStorage('menu_link_content')->load($id) and
dropping the property resolves it.

Note that $menuLinkContentStorage is protected, so removing it outright is
an API change for any subclass that uses it. Options:

a. Remove it in 3.0.x and mention it in the release notes - the property is
not documented as an extension point.
b. Deprecate it now, keep it populated, and remove it in 4.0.x. This leaves
the PHPStan error in place until then, so it would need a targeted
@phpstan-ignore-next-line with a reference to this issue.

Option (a) is preferred unless there is a known subclass relying on it.

Remaining tasks

- Decide between (a) and (b) for the storage property.
- Apply both fixes.
- Confirm the phpstan job is green.
- Consider removing allow_failure from the phpstan job afterwards, so
future regressions are actually visible.

User interface changes

None.

API changes

Depends on the option chosen for $menuLinkContentStorage; see above.

Command icon 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:

Comments

fabianderijk created an issue. See original summary.

fabianderijk’s picture

Status: Active » Fixed

phpstan errors are fixed. Marking as fixed

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

  • fabianderijk committed 91946c6e on 3.0.x
    Issue #3619679: Fix the two PHPStan errors in RestMenuItemsResource