- ๐ณ๐ฟNew Zealand quietone
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.
- First commit to issue fork.
- Merge request !9498Issue #2982582: Added `(optional)` before the severity in the return of docblock. โ (Open) created by nexusnovaz
- Status changed to Needs review
about 1 month ago 11:43am 15 September 2024 - ๐ฌ๐งUnited Kingdom nexusnovaz
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. - Status changed to RTBC
about 1 month ago 1:59pm 16 September 2024 - Status changed to Needs work
about 1 month ago 2:16pm 16 September 2024 - ๐ฌ๐งUnited Kingdom joachim
A parameter marked 'optional' need to say what the default will be.
- ๐ฌ๐งUnited Kingdom nexusnovaz
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
- ๐ฌ๐งUnited Kingdom joachim
Add something like "If omitted [say what happens]."
- Status changed to Needs review
about 1 month ago 6:40pm 16 September 2024 - ๐ฌ๐งUnited Kingdom nexusnovaz
Hi, MR !9498 should now be ready for review.
Thanks
- Status changed to Needs work
about 1 month ago 1:13pm 19 September 2024 - Status changed to Needs review
about 1 month ago 9:16am 20 September 2024 - ๐ฌ๐งUnited Kingdom nexusnovaz
Hopefully i understood #31 correctly. Added the small change! Please review
- Status changed to RTBC
about 1 month ago 1:29pm 20 September 2024 - ๐บ๐ธUnited States smustgrave
Tweaked slightly in the editor but looks good to me.
- ๐ฌ๐งUnited Kingdom nexusnovaz
I see now @smustgrave. Cheers and apologies!
- ๐ณ๐ฟNew Zealand quietone
I read this issue, the MR and the related issue. I think this is an improvement. Leaving at RTBC
- ๐ณ๐ฟNew Zealand quietone
Actually, I am mistaken. A change is needed here. The default value is REQUIREMENT_INFO, see
- ๐ฎ๐ณIndia arunkumark Coimbatore
As per the comment, I made a minor change to the default value in the comment section.
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.
- Merge request !9848Added before the severity in the return of docblock โ (Open) created by arunkumark
- ๐ฎ๐ณIndia arunkumark Coimbatore
arunkumark โ changed the visibility of the branch 2982582-hookrequirements-doesnt-say to hidden.
- ๐ฎ๐ณIndia arunkumark Coimbatore
Created MR against 11.0.x, Now able to review.
- ๐บ๐ธUnited States smustgrave
Why open a new branch when the previous was pointing to the correct target?
- ๐ฎ๐ณIndia arunkumark Coimbatore
arunkumark โ changed the visibility of the branch 2982582-hookrequirements-doesnt-say to active.
- ๐ฎ๐ณIndia arunkumark Coimbatore
arunkumark โ changed the visibility of the branch 2982582-hookrequirements-doesnt-say-d11 to hidden.
- ๐ฎ๐ณIndia arunkumark Coimbatore
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.
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.
- ๐ธ๐ฌSingapore anish.a Singapore
anish.a โ made their first commit to this issueโs fork.
- ๐ธ๐ฌSingapore anish.a Singapore
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.
- ๐ฌ๐งUnited Kingdom joachim
> 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.)
- ๐ฌ๐งUnited Kingdom nexusnovaz
An unrealted test was failing, but after a rerun, its all green now. Marking as needs review unless i've missed something else?
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 ----------------------------------------------------------------------------------------------------
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.
- ๐ฌ๐งUnited Kingdom nexusnovaz
Another issue with phpstan. has less information than #61
---------------------------------------------------------------------------------------------------- Running PHPStan on changed files. PHPStan: failed ----------------------------------------------------------------------------------------------------
I am unsure how to fix this
- ๐ฎ๐ณIndia arunkumark Coimbatore
Seems like a false positive moving to NR.