The attached patch;-

  • Adds two classes to the markup
    'number':
    So that numbers can be right aligned
    'warning':
    So non-zero values of certain keys, can be highlighted
  • Provides the css styles to render appropriately

I was prompted to do this after finding incorrect description http://drupal.org/node/107375.

Patch should apply with -p0, possibly against HEAD, but was made against 6.0 RC2.

CommentFileSizeAuthor
#31 status.patch726 bytescatch
#25 issue-212233.patch4.18 KBlilou
#18 sql_status_7.x.patch4.36 KBlilou
#16 numeric_column.patch955 byteszeta ζ
#17 sql_status_.patch4.32 KBzeta ζ
#15 sql_status_.patch4.25 KBzeta ζ
#13 SQL.patch3.32 KBzeta ζ
#12 sql_status.patch3.21 KBzeta ζ
#8 pretty_status.patch1.59 KBzeta ζ
#7 pretty_status.patch1.55 KBzeta ζ
#5 sytem.admin_.inc_.patch833 byteszeta ζ
#5 SQL_report.png32.17 KBzeta ζ
#3 SQL | localhost_1201191514420.png79.62 KBzeta ζ
#1 pretty_status.patch1.49 KBzeta ζ
pretty_status.patch1.47 KBzeta ζ

Comments

zeta ζ’s picture

zeta ζ commented
StatusFileSize
new pretty_status.patch1.49 KB

Added another key.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented

How would the improved interface look compared to the old?

zeta ζ’s picture

zeta ζ commented
StatusFileSize
new SQL | localhost_1201191514420.png79.62 KB

The Value column: right aligned.
Non-zero values of ‘should be zero’ highlighted.

Both done with classes, so that themes can override.
See attached for default.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Needs review » Needs work

I know the goal here is to get attention to the bad value, but this looks clunky. The yellow box is quite big around the number (I know it is the table cell, but it looks odd).

zeta ζ’s picture

zeta ζ commented
Status: Needs work » Needs review
StatusFileSize
new SQL_report.png32.17 KB
new sytem.admin_.inc_.patch833 bytes

Yes, I agree, but I wanted to avoid extra markup.

What about this – I’ve borrowed an icon from watchdog.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Needs review » Needs work

Looks much better IMHO, but you posted the wrong patch.

zeta ζ’s picture

zeta ζ commented
Status: Needs work » Needs review
StatusFileSize
new pretty_status.patch1.55 KB

Sorry…

zeta ζ’s picture

zeta ζ commented
StatusFileSize
new pretty_status.patch1.59 KB

This is better, as it applies class .warning to whole row, the way watchdog does it.

Haven’t used the (not so) yellow background that watchdog applies to the row, so looks the same as in #5 *.png .

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Needs review » Needs work

tr .number seems buggy, since it would apply to the row, while your intention is to apply it to a cell, right?

zeta ζ’s picture

zeta ζ commented

tr .number is the class of a td within a row. tr td.number would be more explicit, but unnecessary, and I think I remember reading that it is less efficient.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Needs work » Needs review

Indeed, sorry for missing the space there :)

zeta ζ’s picture

zeta ζ commented
Version: 6.0-rc2 » 6.x-dev
StatusFileSize
new sql_status.patch3.21 KB

I’ve re-worked this and it will now apply against HEAD (not sure if that is 6.x-dev or 7.x-dev). Will also apply to D6.2 with offsets (-49) if you use patch -p1 -F3 .

zeta ζ’s picture

zeta ζ commented
Version: 6.x-dev » 7.x-dev
StatusFileSize
new SQL.patch3.32 KB

Re-rolled against HEAD. I can roll against 6.3 or -dev if needed. Fairly easy to port to 5 too.

I kmow it’s only a task, but it would be great to get this in as http://drupal.org/node/107375 is already there: picked that one up after a year languishing as ‘needs work’.

dries’s picture

dries commented

This patch seems to generate NOTICES when I try it.

zeta ζ’s picture

zeta ζ commented
StatusFileSize
new sql_status_.patch4.25 KB

Sorry Dries, mended now :-) using isset() (and E_ALL). Obviously I’m still getting used to PHP’s foibles.

Note:
I’ve just posted http://drupal.org/node/284721 which also relates to this code
zeta ζ’s picture

zeta ζ commented
StatusFileSize
new numeric_column.patch955 bytes

This patch could be applied after the above (Edit: or the below). It moves the ’right alignment’ to system.css, from admin.css, so that tables in general can have numeric columns styled by adding class="numeric" to each <td> or <th>

zeta ζ’s picture

zeta ζ commented
StatusFileSize
new sql_status_.patch4.32 KB

Re-roll after commit of http://drupal.org/node/284721

lilou’s picture

lilou commented
StatusFileSize
new sql_status_7.x.patch4.36 KB

Re-roll;

lilou’s picture

lilou commented
Status: Needs review » Needs work

Patch #18 no longer applies against CVS/HEAD.

dries’s picture

dries commented

Can we get a fresh screenshot too? :)

lilou’s picture

lilou commented
Title: Improve UI of SQL status report » Kill SQL status report ?

Perhaps it's time to kill system_sql() now drupal support SQLite and Postgresql ?

/**
 * Menu callback: return information about the database.
 */
function system_sql() {

  $result = db_query("SHOW STATUS");
  while ($entry = db_fetch_object($result)) {
    // 'SHOW STATUS' returns fields named 'Variable_name' and 'Value',
    // case is important.
    $data[$entry->Variable_name] = $entry->Value;
  }

return Notice: Undefined property: stdClass::$Value in system_sql() (line 1851 of D:\Serveur\www\drupal\7.x\modules\system\system.admin.inc).

lilou’s picture

lilou commented
Title: Kill SQL status report ? » admin/reports/status/sql : kill SQL status report ?
catch’s picture

catch
he/him
Primary language English
commented

We should remove it. If we end up adding back in a database report at a later date all well and good.

lilou’s picture

lilou commented
Assigned: zeta ζ » lilou

OK, i take it.

lilou’s picture

lilou commented
Title: admin/reports/status/sql : kill SQL status report ? » admin/reports/status/sql : remove SQL status report
StatusFileSize
new issue-212233.patch4.18 KB
lilou’s picture

lilou commented
Status: Needs work » Needs review

see also #423926: Restore database information screen

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

Here's how it looks at the moment.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Ok, let's nuke it -- it could be a good feature for devel module. Committed. Thanks!

damien tournoud’s picture

damien tournoud commented
Status: Fixed » Needs work

Please also nuke the db_status_report() call in system_requirements().

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Needs review
StatusFileSize
new status.patch726 bytes
lilou’s picture

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

Looks good.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Committed!

Status: Fixed » Closed (fixed)

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