One of the things the recent admin/settings re-order patch didn't address was the fact that some of the 'settings' there weren't settings at all, but were in fact checks or reports.

For example, the Unicode fieldset reported on which string handling mode was used, as well as if there were errors with the mbstring installation. The cron fieldset only reported the last run cron time. A lot of these items were not only out-of-place, but also useless for the admin of a perfectly working Drupal site.

So, I moved these items off to a separate status report at admin/status and focused more on the errors themselves. If there are errors detected with the installation, they are reported (e.g. cron has never run). Advice to the admin (e.g. enable mbstring for better internationalization) is reported as a simple notice.

I also included some simple version reporting information about the web environment (server, php, sql versions). It is e.g. handy for people to copy/paste around.

Finally, to increase the visibility of the status report, I tweaked it so it can be run in the background. That way, we can perform these checks when the admin enters the administration section and let them know if there are any problems without blasting them with technical information:

This might seem a bit weird now, but e.g. when we convert /admin to a dashboard, this message will be very much at home.

Note that the status report is implemented as a themed form. This has several advantages:

  • We use the standard form error mechanism to report, theme and display errors.
  • Modules can insert extra status info through hook_form_alter().
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Dries’s picture

Looks like an improvement to me. :)

It could also be the home of the "continious requirement checking" results.

The code looks good. The only thing that looks a bit hack-ish is the newly introduced $wipe parameter to form_set_error and the corresponding calls (eg. form_set_error(NULL, '', TRUE)) ... would be nice if that could be done in a slightly more elegant way.

Dries’s picture

Why don't you show the Unicode status message in the table? Both the Unicode information and the crontab information is "scattered". Part of it is shown at the top, part of it is shown in the table. If the status report grows, this might be a little bit of a problem. Maybe there is room for improvement in the way this information is presented.

One thing that might not be clear is the impact or urgency of the problem. Not having cron support is not necessarily a problem. With all the 'red', it sure looks like a big problem.

Also, it begs the question: what to do with the checking of the files-directory?

And more recently, with the writability of the settings.php.

Maybe we can flesh this out a little more? It certainly has a lot of potential.

Steven’s picture

Dries: The idea is, we don't output verbose error messages until something needs to be done by the admin. Before this patch, there was always a verbose message both about cron and unicode on admin/settings. I think that there is often too much text on Drupal screens. When nothing is wrong, you get the following simple screen:

Only local images are allowed.

Think of this split as "What you need to know now" (error/advice messages) vs "What you might want for reference" (technical info, e.g. when asking for support), i.e. splitting information up by task rather than origin.

Also, severity-wise I do think this is on target. Maybe the error styling needs to be less aggressive (I already did this for maintenance.css in the installer), but if cron is broken, tables will not be cleaned periodically, search will not work, feeds will not aggregate, etc. An admin who wants to run with broken cron can still do so, but cannot claim ignorance.

Steven’s picture

Bah. Will project issue text filtering ever not suck?

Only local images are allowed.
http://acko.net/dumpx/status-report-3.png

merlinofchaos’s picture

I definitely +1 this. This would make a good 'block' on the admin page http://drupal.org/node/72079 if that patch makes it in.

Steven’s picture

FileSize
14.62 KB

I'm not sure about making it a block. Error messages could still go there, but advice would be a bit excessive.

Updated the patch to HEAD. It is now an item in the admin/logs block. I also renamed the on/off-line toggle "site status" settings to "site maintenance" to reduce confusion between the two.

Finally, I tweaked the styling of errors to be less aggressive and more readable.

Steven’s picture

FileSize
16.38 KB

Changed weight of status report item (merlinofchaos).

merlinofchaos’s picture

To summarize IRC discussion:

I'm pretty happy with this, in general. While I lean a little bit toward keeping the information in the table, I've been convinced by Steven that putting the important "you should take action!!" messages in the error box above is a good idea. It brings attention to them that may not be seen in the table, especially if the table grows.

Steven’s picture

FileSize
20.35 KB

Here's an alternate patch that puts all the information in the table. I think it's messier and harder to interpret than using messages. Plus, the implementation is a bit messier now that messages have to be passed back manually rather than using form_set_error() and drupal_set_message().

Compare:
http://acko.net/dumpx/status-report-1.png
http://acko.net/dumpx/status-report-4.png

(note that the last patch also has the same less aggressive error styling)

merlinofchaos’s picture

If it makes the code messier, then I'm not for the alternate patch.

Dries’s picture

Looking at the screenshots (not looking at the code), I like status-report-4.png best. Good job, Steven. I'll try to review the code later, after we got some early feedback about the presentation.

(Web Server -> Web server)

rickvug’s picture

#4 is definitely more inviting, although part of this might just be how the wider table just looks better to my eye.

kika’s picture

Offtopic: Subscribing. Why not the project module can distinguish between issue addition and pure bookmarking? And how can I unsubscribe later?

m3avrck’s picture

FileSize
7.25 KB

I agree with Dries, the table layout makes the most sense usability wise, keep everything together.

However, that interface is still a bit confusing, so attached is what I propose, rearranged a bit, with new "OK", "FAIL", and "ALERT" flags to the user, properly colored with CSS classes too (for those themers that want to use icons).

We can then use some JQuery magic later to show/hide the details for each message.

Dries’s picture

m3avrck: nice! I think your design has the most potential. :)

Steven’s picture

Status of this patch: the plan was to upgrade this code to support install-time requirements later, and to integrate them into the status page. Unfortunately, due to the way the install system works, it meant changing a lot of code around. Meaning: this patch was useless as a waystation as it would be rewritten completely.

So I went around and made the changes in a tree locally. The principle works well already, but there are some practical issues. Right now I'm waiting for the following patch to hit:
http://drupal.org/node/76802

forngren’s picture

Please note that the cron status page has been extended; http://drupal.org/node/76958

moshe weitzman’s picture

the t() patch has landed. lets get this one in this week, if possible.

merlinofchaos’s picture

On the IRC, eaton was talking about a hook_module_status()

That seems a perfect candidate to be included here, while there's still time to do it.

A hook means that any module can participate in this, and the status page can become a serious portal for "What things need to be done on your site". Which is an administrator's dream.

eaton’s picture

My discussion of said hook grew out of the efforts to refactor the module administration page. It occurred to me that it would be REALLY helpful for modules to be able to provide information on their current 'status', or on things that need attention, etc. Three examples I thought of:

  1. upload.module: You need to create the /files directory, *click here* to configure it.
  2. comment.module: 50 comments are waiting to be approved. *click here* to see them.
  3. (hypothetical) security.module: Someone may be trying to crack the admin password. *click here* to see the list of failed login attempts.

Etc etc...

I'm looking at this patch a bit closer to see how it could be used for that...

Steven’s picture

Title: Split off status report messages » Install-time and run-time requirements checking
FileSize
33.5 KB

Here's a first version of the upgraded requirement/status system. Changes:

  • I changed it from merely a system of notices/info to a requirements checking system. Requirements can be checked at install-time, run-time or both. We'll use it to prevent broken installs (old PHP, not enough memory, ...) and to notify the admin of maintenance tasks (e.g. cron has not run, there is a new version of Drupal out, settings.php is still writable, ...).
  • Requirements are simple checks that give you a severity level back: ok, warning or error. Ok means that requirements are satisfied. Warnings are given about non-critical issues (e.g. not using mbstring for Unicode) whereas errors are serious problems.
  • Requirements are defined per module in module.install's hook_requirements($phase). Core requirements are in system.install, but any module can add requirements for install-time, run-time or both.
  • The admin/status page has been changed into a requirements report, which will check the site's operation when you hit it. If there are errors, this message is pushed even to the main admin page ("One or more problems were detected. See: status report").

Screenshots:
http://acko.net/dumpx/requirements-report.png
http://acko.net/dumpx/requirements-install-error.png
http://acko.net/dumpx/requirements-install-warning.png

Here's an example of some trivial requirement that is checked both at run-time and install time:

function gargamel_requirements($phase) {
  global $smurfs;

  $t = function_exists('install_main') ? 'st' : 't';

  $requirements = array();

  if ($smurfs > 100) {
    $requirements['gargamel'] = array(
        'title' => $t('Smurfs'),
        'value' => $smurfs,
        'severity' => REQUIREMENT_ERROR,
        'description' => $t('Gargamel module cannot be installed because of smurf infestation.'),
    );
  }
  return $requirements;
}

Note that thanks to the t() placeholder patch, we can use a variable function name $t to call either t() or st() when it's respectively install-time or run-time.

I tested this patch locally and it seems to work okay. Please give it a try. More requirements and checks are trivial to add in the future (see system.install).

moshe weitzman’s picture

To my eyes, that install warning is too harsh. Installing is scary enough without issueing warnings that aren't that very important. I would prefer that site admins learn abou these on teir status report. my .02

other screenshots look great. i will try to test soon.

Chris Johnson’s picture

Patch code and screen shots look good. I agree with Moshe that only errors need be displayed at install time. Leave the warnings for the admin section.

Steven’s picture

FileSize
32.38 KB

Removed the install-time warnings (only errors are shown now).

drewish’s picture

few problems with your last patch:
- hunks failed on install.php and includes/install.inc but it seemed to be because a change had already been applied.
- unicode_settings() was replaced by unicode_requirements() we should probably drop system_unicode_settings() and the menu item from system_menu() too.
- misc/watchdog-ok.png is missing... maybe you could attach it to this issue?
- i'm not able to open admin/settings/site-information or admin/settings/site-maintenance, something is crashing mysql server and causing apache to restart... might not be related to this patch.

Dries’s picture

In addition to what others reported:

1. Small feature request: it would be handy if there was a phpinfo()-link to get easy access to all of your PHP settings.

2. Personally, I find the $t = function_exists('install_main') ? 'st' : 't'; rather sloppy. Can't we make t() smarter or something?

3. Please move your example into the PHPdoc. Maybe remove the smurf stuff and replace it with a real example (with practical value).

4. 'not protected from modifications' sounds a little weird. Maybe use world-writable?

5. Most sites need mail/SMTP-functionality. Is there an easy way to figure out whether mail() is properly configured?

6. No requirement checking for MySQL? Sounds important enough to check at install time ...

drewish’s picture

if i can make a feature request at the same time, add a _status() function that displays the maximum posted file upload sizes.

Steven’s picture

Even nicer would be that people would add some tests instead of asking me to code it :P.

drewish’s picture

humm, well i'd intended to write my own feature but now i'm trying that patch on another machine and /admin* and /node/add/* are both returning empty pages. there's no PHP error but something's crashing apache.

Dries’s picture

7. You'll probably want to remove the dedicated 'cron status' page (q=admin/settings/cron-status) and extend yours with the ability to run cron manually.

Dries’s picture

As a developer, and lover of clean code, the $t = function_exists('install_main') ? 'st' : 't'; really turns me down though. Did we try benchmarking a cleaner solution? I think we should, and probably, before everyone starts making these _requirement hooks. If you provide a simple patch, I'd be happy to benchmark it and we can conclude what to do with it.

From all remarks, these are the ones I think are important:

1. Please move your example into the PHPdoc. Maybe remove the smurf stuff and replace it with a real example (with practical value). Documentation matters.

2. You'll probably want to remove the dedicated 'cron status' page (q=admin/settings/cron-status) and extend yours with the ability to run cron manually. Right now, this functionality is duplicated.

The other issues are less important, IMO, and can be refined afterwards.

Steven’s picture

FileSize
35.71 KB

I moved the cron stuff to the status page. The documentation will definitely go in properly, but it should go into the contrib hook docs for hook_requirements(), not in this patch.

As for the $t stuff, I personally find the $t approach very good given the situation. Only the fact that we repeat the function_exists() call is a bit annoying. Would switching to a $t = drupal_get_t(); or something make it better?

Adding some switching logic to t() itself would probably be a significant hit though. Remember that unlike the placeholder patch, this change would apply to every single t() call, not just those t() calls that use placeholder "%arguments" (which is actually a very small minority of all t() calls per page).

asimmonds’s picture

FileSize
35.3 KB

Patch didn't apply properly, so have re-rolled against current HEAD. No other changes done.

asimmonds’s picture

FileSize
35.37 KB

Found a little bug, drupal_check_module() didn't return anything so couldn't install any other modules.

asimmonds’s picture

Status: Needs review » Needs work

What is the change in drupal_add_js() required for? In my environment, drupal_add_js() is called recursively until PHP dies.

Steven’s picture

Without that change, drupal.js is added to every single page, even if there is no javascript used on the page. This caused problems with the installer. It works fine here.

asimmonds’s picture

Status: Needs work » Needs review
FileSize
35.39 KB

In this patch, have moved the ...drupal_add_js('misc/drupal.js', 'core');... after the switch() in drupal_add_js().

If this is before the switch(), $javascript['header']['core']['misc/drupal.js']) is never set and drupal_add_js() goes into a recursive loop.

Dries’s picture

FileSize
37.93 KB

I added some additional information to this patch:

  • Show some PHP information
  • Show some MySQL information

The information is accessible by clicking the version numbe (intentionally somewhat hidden).

Also fixed up a number of small things. Most notably, some things were check_plain()'ed twice, or at different levels. For example db_status_report() returned check_plain()ed data but the PHP version was not. Then, on output, certain things are check_plain()ed again, while others are not.

Steven: I tried to fix this a little but you'll want to double check these changes.

Except for the $t() stuff, this looks ready to me. The $t() stuff is no showstopper so feel free to commit after one more review.

drewish’s picture

asimmonds, your javascript fix got all the admin pages working for me. this is looking really good.

Steven’s picture

Status: Needs review » Fixed

I gave this patch a final look-over and committed it to HEAD (after an ok from Dries):

  • Fixed inconsistencies with check_plain()
  • Don't link to PHP/SQL report pages during install
  • Added get_t() to make the $t trick a bit cleaner.
  • Added mysql/pgsql version checking.
  • Added requirements checking when a module is installed. Errors prevent the installation.
Anonymous’s picture

Status: Fixed » Closed (fixed)