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().
Comment | File | Size | Author |
---|---|---|---|
#38 | requirement-extended.patch | 37.93 KB | Dries |
#37 | requirements_6.patch | 35.39 KB | asimmonds |
#34 | requirements_5.patch | 35.37 KB | asimmonds |
#33 | requirements_4.patch | 35.3 KB | asimmonds |
#32 | requirements_3.patch | 35.71 KB | Steven |
Comments
Comment #1
Dries CreditAttribution: Dries commentedLooks 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 toform_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.Comment #2
Dries CreditAttribution: Dries commentedWhy 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.
Comment #3
Steven CreditAttribution: Steven commentedDries: 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:
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.
Comment #4
Steven CreditAttribution: Steven commentedBah. Will project issue text filtering ever not suck?
http://acko.net/dumpx/status-report-3.png
Comment #5
merlinofchaos CreditAttribution: merlinofchaos commentedI definitely +1 this. This would make a good 'block' on the admin page http://drupal.org/node/72079 if that patch makes it in.
Comment #6
Steven CreditAttribution: Steven commentedI'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.
Comment #7
Steven CreditAttribution: Steven commentedChanged weight of status report item (merlinofchaos).
Comment #8
merlinofchaos CreditAttribution: merlinofchaos commentedTo 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.
Comment #9
Steven CreditAttribution: Steven commentedHere'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)
Comment #10
merlinofchaos CreditAttribution: merlinofchaos commentedIf it makes the code messier, then I'm not for the alternate patch.
Comment #11
Dries CreditAttribution: Dries commentedLooking 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)
Comment #12
rickvug CreditAttribution: rickvug commented#4 is definitely more inviting, although part of this might just be how the wider table just looks better to my eye.
Comment #13
kika CreditAttribution: kika commentedOfftopic: Subscribing. Why not the project module can distinguish between issue addition and pure bookmarking? And how can I unsubscribe later?
Comment #14
m3avrck CreditAttribution: m3avrck commentedI 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.
Comment #15
Dries CreditAttribution: Dries commentedm3avrck: nice! I think your design has the most potential. :)
Comment #16
Steven CreditAttribution: Steven commentedStatus 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
Comment #17
forngren CreditAttribution: forngren commentedPlease note that the cron status page has been extended; http://drupal.org/node/76958
Comment #18
moshe weitzman CreditAttribution: moshe weitzman commentedthe t() patch has landed. lets get this one in this week, if possible.
Comment #19
merlinofchaos CreditAttribution: merlinofchaos commentedOn 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.
Comment #20
eaton CreditAttribution: eaton commentedMy 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:
Etc etc...
I'm looking at this patch a bit closer to see how it could be used for that...
Comment #21
Steven CreditAttribution: Steven commentedHere's a first version of the upgraded requirement/status system. Changes:
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:
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).
Comment #22
moshe weitzman CreditAttribution: moshe weitzman commentedTo 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.
Comment #23
Chris Johnson CreditAttribution: Chris Johnson commentedPatch 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.
Comment #24
Steven CreditAttribution: Steven commentedRemoved the install-time warnings (only errors are shown now).
Comment #25
drewish CreditAttribution: drewish commentedfew 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.
Comment #26
Dries CreditAttribution: Dries commentedIn 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 ...
Comment #27
drewish CreditAttribution: drewish commentedif i can make a feature request at the same time, add a _status() function that displays the maximum posted file upload sizes.
Comment #28
Steven CreditAttribution: Steven commentedEven nicer would be that people would add some tests instead of asking me to code it :P.
Comment #29
drewish CreditAttribution: drewish commentedhumm, 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.
Comment #30
Dries CreditAttribution: Dries commented7. 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.
Comment #31
Dries CreditAttribution: Dries commentedAs 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.
Comment #32
Steven CreditAttribution: Steven commentedI 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).
Comment #33
asimmonds CreditAttribution: asimmonds commentedPatch didn't apply properly, so have re-rolled against current HEAD. No other changes done.
Comment #34
asimmonds CreditAttribution: asimmonds commentedFound a little bug, drupal_check_module() didn't return anything so couldn't install any other modules.
Comment #35
asimmonds CreditAttribution: asimmonds commentedWhat is the change in drupal_add_js() required for? In my environment, drupal_add_js() is called recursively until PHP dies.
Comment #36
Steven CreditAttribution: Steven commentedWithout 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.
Comment #37
asimmonds CreditAttribution: asimmonds commentedIn 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.
Comment #38
Dries CreditAttribution: Dries commentedI added some additional information to this patch:
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.
Comment #39
drewish CreditAttribution: drewish commentedasimmonds, your javascript fix got all the admin pages working for me. this is looking really good.
Comment #40
Steven CreditAttribution: Steven commentedI gave this patch a final look-over and committed it to HEAD (after an ok from Dries):
get_t()
to make the $t trick a bit cleaner.Comment #41
(not verified) CreditAttribution: commented