Problem/Motivation

Contrib modules running PHPStan on level 6 get errors due to the requirements array not being described.

Proposed resolution

Add a PHPDoc array shape @phpstan-type that can be imported where necessary.

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#16 3565560-nr-bot_gg5z9ze4.txt98 bytesneeds-review-queue-bot

Issue fork drupal-3565560

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:

About issue forks

Comments

mondrake created an issue. See original summary.

mondrake opened merge request !14215

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Active » Needs review
mfb’s picture

mfb
they or he
commented
Status: Needs review » Needs work

I think that's not the correct interface to use? I get t('') instanceof \Drupal\Core\Entity\TranslatableInterface === FALSE

Maybe just string|\Stringable is fine, not my area of expertise though.

I guess you're implying that all the keys are optional? Which isn't what the existing docs say, but I've not tested it out.

mfb’s picture

mfb
they or he
commented

Some more type questions:

It looks to me like value and description are permitted to be render arrays, not just Stringable? (when testing this out in a legacy requirements hook)

I think value could also reasonably be a numeric type? In case someone wants to print a number without units.

mstrelan’s picture

mstrelan commented

If only we had made this a value object in #3490843: Create class to replace hook_requirements('install') we wouldn't need this. Maybe we still can, but that doesn't block this issue.

mondrake’s picture

mondrake
Location 🇮🇹
commented

Thanks for review.

I guess you're implying that all the keys are optional?

It seems that "title" can be avoided during installation. But probably not during update/runtime, so in order not to have two different types for install vs runtime I think better keep it optional. The "weight" key seems no longer used, but I see it appearing.

value and description are permitted to be render arrays

yes, done

I think value could also reasonably be a numeric type

not what the api file says... tried here to align definitions between the API file and the interface.

FWIW I think MR!14215 is how this should be represented, but coding standards and probably the API module are not there. Shrug...

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Needs review
dcam’s picture

dcam commented
Status: Needs review » Needs work
Related issues: +#2505499: Explicitly support render arrays in hook_requirements()

Needs work for a rebase.

The "weight" key seems no longer used, but I see it appearing.

Should we note above the RequirementInfo definition in InstallRequirementsInterface and module.api.php that the key is no longer used and is included for legacy purposes? Otherwise there's no information on why it's included in the array shape, but not in the docs. I had to come back here to find out why while reviewing.

Related, but not in-scope: is hook_requirements() in module.api.php supposed to have a deprecation annotation?

I'm adding a related issue where some of these same issues were brought up.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Needs review

Rebased.

Should we note above the RequirementInfo definition in InstallRequirementsInterface and module.api.php that the key is no longer used and is included for legacy purposes?

Sure. Done.

Thanks!

dcam’s picture

dcam commented
Status: Needs review » Reviewed & tested by the community

Cool. That's all I could find to comment on. It looks good to me.

longwave made their first commit to this issue’s fork.

mondrake’s picture

mondrake
Location 🇮🇹
commented

Personally, I do not find it easier to read... especially if we get to finalise #3360160: Stop using FQCN in PHPDoc annotations. Not going to argue though.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

I suppose we really need an issue either in Coder or PHPCS to handle @phpstan-type better?

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3565560-nr-bot_gg5z9ze4.txt98 bytes

The Needs Review Queue Bot tested this issue. The merge request has merge conflicts and cannot be merged. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

mondrake’s picture

mondrake
Location 🇮🇹
commented

Rebased. If #3360160: Stop using FQCN in PHPDoc annotations were in, this could be simplified to 

 * @phpstan-type RequirementInfo array{
 *   'title'?: TranslatableMarkup,
 *   'value'?: TranslatableMarkup|array<string, mixed>|string,
 *   'description'?: TranslatableMarkup|array<string, mixed>,
 *   'severity'?: RequirementSeverity,
 *   'weight'?: int,
 * }

way more readable IMHO.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Reviewed & tested by the community

.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

Why are we adding weight back. Let's not let's remove it and then PHPStan will tell people to remove it which is great.

FYI the workspace requirement in this change could be removed that's not been relevant since Drupal 8 :).