hook_requirements() doesn't say that severity is optional, or what the default is

In general, I disagree with postponing a documentation issue on a decision that will be made in the future. I think the documentation should reflect the current state of the code. Plus the other issue has not had an update to the patch there in 9 years. I see no gain in waiting.

Therefor I am un-postponing.

Assuming I haven't misunderstood, this should be ready for review. Please see MR !9498.

I've added the (optional) before severity in the docblock.

Seems straight forward.

A parameter marked 'optional' need to say what the default will be.

Hey @joachim,

How should i mark the default? Would it be (default) before the option? Does the default need to be the first in the list? (default) at the end of the item? I couldn't find anything in the coding standards, unsure if maybe i've missed it.

Thanks

Add something like "If omitted [say what happens]."

Hi, MR !9498 should now be ready for review.

Thanks

Small suggestion to simplify it

Hopefully i understood #31 correctly. Added the small change! Please review

Tweaked slightly in the editor but looks good to me.

I see now @smustgrave. Cheers and apologies!

No worries at all! Thanks!

I read this issue, the MR and the related issue. I think this is an improvement. Leaving at RTBC

Actually, I am mistaken. A change is needed here. The default value is REQUIREMENT_INFO, see

• #665790-291: Redesign the status report page →
• https://git.drupalcode.org/project/drupal/-/blame/11.x/core/modules/syst...

As per the comment, I made a minor change to the default value in the comment section.

LGTM.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. 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.

arunkumark → changed the visibility of the branch 2982582-hookrequirements-doesnt-say to hidden.

Created MR against 11.0.x, Now able to review.

Why open a new branch when the previous was pointing to the correct target?

arunkumark → changed the visibility of the branch 2982582-hookrequirements-doesnt-say to active.

arunkumark → changed the visibility of the branch 2982582-hookrequirements-doesnt-say-d11 to hidden.

I faced the issue of creating MR against the 11.x branch. Now it is working to update the MR

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. 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.

MR needs a small tweak, but is nearly there!

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. 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.

anish.a → made their first commit to this issue’s fork.

Adding for review again after rebase.

There are test failures. I am unsure about how to fix it.

Re #37 🐛 hook_requirements() doesn't say that severity is optional, or what the default is RTBC :

Actually, I am mistaken. A change is needed here. The default value is REQUIREMENT_INFO, see

#665790-291: Redesign the status report page
https://git.drupalcode.org/project/drupal/-/blame/11.x/core/modules/syst...

The linked code still matches what was mentioned in #2 🐛 hook_requirements() doesn't say that severity is optional, or what the default is RTBC , and should probably be documented as such:

      $severity = $severities[REQUIREMENT_INFO];
      if (isset($requirement['severity'])) {
        $severity = $severities[(int) $requirement['severity']];
      }
      elseif (defined('MAINTENANCE_MODE') && MAINTENANCE_MODE == 'install') {
        $severity = $severities[REQUIREMENT_OK];
      }

Which sounds like undefined severities default to REQUIREMENT_OK while installing, and REQUIREMENT_INFO otherwise.

This should indeed be documented explicitly.

> Which sounds like undefined severities default to REQUIREMENT_OK while installing, and REQUIREMENT_INFO otherwise.

Yup, well spotted!

(Also, the logic in that piece of code could be a lot clearer - I'll file an issue to refactor it.)

(Also, it seems like REQUIREMENT_INFO might only apply during runtime? Again, something for a separate issue.)

An unrealted test was failing, but after a rerun, its all green now. Marking as needs review unless i've missed something else?

lgtm

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. 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.

Some change upstream must have affected the PHPStan check, because the bot failed on this:

----------------------------------------------------------------------------------------------------

Running PHPStan on changed files.


 [ERROR] No files found to analyse.



PHPStan: failed

----------------------------------------------------------------------------------------------------

back to rtbc

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. 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.

Another issue with phpstan. has less information than #61

----------------------------------------------------------------------------------------------------

Running PHPStan on changed files.

PHPStan: failed

----------------------------------------------------------------------------------------------------

I am unsure how to fix this

Seems like a false positive moving to NR.

Looks good!

I think the problem is there might be a bug with the PHPStan config in the NR bot, so it will kick this issue back to NW on the next upstream change. But we'll see.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. 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.

Seems like a false positive. After latest merge moving to NR.

Back to RTBC.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. 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.

Done the rebase to the 11.x

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. 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.

Did some investigation on the bot failures: Looks like PHPStan is only running against files in the MR diff. It's configured to ignore *.api.php files, and if there are no files to scan, it's also configured to exit with an error, so the bot flags that as a failure. (For MR builds, PHPStan runs over all core files, so this situation doesn't come up.)

For the future, I think it makes sense to push this issue back to RTBC whenever the bot sets it to NW, unless there really is a merge conflict to resolve. Hopefully it gets merged soon.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. 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.

You can disable the bot on issues like this with the tag I just added.

quietone → closed merge request !9498

Committed to 11.x and cherry-picked to 11.1.x, 10.5.x, and 10.4.x.

Thanks for the improvement!

Automatically closed - issue fixed for 2 weeks with no activity.