Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Background
System module hijacks the admin/reports/status page to group the output from its hook_requirements implementation into "General System Information". This gives admins a heads-up way to view system-level data about their Drupal install.
Bug Description
Unfortunately, this breaks the other heads-up information display about errors and warnings.
If system_requirements generates an error or warning, the feedback doesn't get grouped where we expect to find it with other non-system errors and warnings. This can lead to significant confusion:
- the "details" section doesn't contain the same number of values listed in the heads-up info.
- the errors and warnings generated by system_requirements are not visually denoted in any way to distinguish them from OK operation.
- if system_requirements is the only module generating warnings or errors, the "details" link doesn't work. There is no #error or #warning element on the page, because system.module hijacks them in the theme layer.
Here's an example demonstrating these issues for the "1 error" scenario:
Suggested resolutions:
Option 1: if system_requirements returns non-OK requirements, do not group them in the heads-up "General" display
Option 2: if system_requirements returns non-OK requirements, show them twice -- once in the "General" display, and once along with the errors and warnings listings, as applicable.
Here's a screenshot of proposed resolution, option 2, as implemented by patch in #2:
| Comment | File | Size | Author |
|---|---|---|---|
| #37 | 2869868-37.patch | 3.71 KB | smustgrave |
| |||
| #37 | interdiff-29-37.txt | 1.5 KB | smustgrave |
Comments
Comment #2
AaronBaumanOption 2 has the most minimal impact, not messing with the "general info" box at all.
Attached patch contains an implementation of that approach.
Comment #3
AaronBaumanHere's a test demonstrating the bug described.
I expect this test to fail against unpatched core, and to pass with the patch from #2
Comment #4
AaronBaumanUpdated issue summary with screenshot from patch in #2
Comment #5
idebr CreditAttribution: idebr at iO commentedComment #7
AaronBaumanAttaching combined patch + test for review
Comment #9
AaronBaumanComment #12
AaronBaumanLet's try a different CSS selector for the tests
Comment #13
AaronBaumanMarking NW until I can get a test to fail the way i expect it to.
Comment #14
AaronBaumanHere's the patch that we expect to fail to demonstrate the current bugginess.
There are 2 new tests:
testErrorElementCount() to confirm that the "N Errors" counter display matches the number of elements displayed under the "Errors" header
testWarningElementCount() to confirm that "N Warnings" counter display matches the number of elements displayed under the "Warnings" header
Comment #16
AaronBaumanOK, still not quite right.
One more try.
Comment #18
AaronBaumanGreat, this test is failing as asserted in the OP.
The "Errors" listing has 2 items, but the counter does not say "2 Errors"
The "Warnings" listing has 2 items, but the counter does not say "2 Warnings"
Attached patch includes the failing test from #16, as well as the fix described by Suggested Resolutions Option 2 in OP.
Comment #28
larowlanComment #29
immaculatexavier CreditAttribution: immaculatexavier as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedRerolled the Patch against 9.4.x. Please review
Comment #30
immaculatexavier CreditAttribution: immaculatexavier as a volunteer and at Srijan | A Material+ Company for Drupal India Association commented#29 Rerolled the Patch against 9.4.x. Please review
Comment #31
quietone CreditAttribution: quietone at PreviousNext commented@immaculatexavier, thanks for the interest in this issue. To help reviewers always add an interdiff, or a diff, whichever is appropriate. There are instructions for creating an interdiff . If you think a diff is not needed, add a comment stating why. Thanks!
Comment #32
immaculatexavier CreditAttribution: immaculatexavier as a volunteer and at Srijan | A Material+ Company for Drupal India Association commented@quietone,
Sorry for the inconvenience, I do agree interdiff is necessary.
Attached the diff for rerolled patch #29
Comment #33
Rinku Jacob 13 CreditAttribution: Rinku Jacob 13 at Zyxware Technologies commentedHi, I need some clarification about the issue, Because The "details" section contains the same number of Errors and warnings listed in the heads-up info before and after the patch was applied. But the number Checked value is different in both heads-up and detail info. After the patch, I counted the number of checked values. I got the value as 24. but in heads-up, it's showing as 23, Can anyone please check about that?
Comment #36
smustgrave CreditAttribution: smustgrave at Mobomo commentedThis issue is being reviewed by the kind folks in Slack, #need-reveiw-queue. We are working to keep the size of Needs Review queue [2700+ issues] to around 200, following Review a patch or merge require as a guide.
D10 version needed
At this time we would need a D10.1.x patch or MR for this issue.
Comment #37
smustgrave CreditAttribution: smustgrave at Mobomo commentedRerolled for D10.1
Fixed some deprecation calls to REQUEST_TIME.
Comment #38
catchSmall fix with plenty of new test coverage. Looks good.
Comment #39
lauriiiCommitted 2d6ae27 and pushed to 10.1.x. Thanks!