This patch cleans up the entire module. It removes redundant code and while doing that also fixes a bug (a separate issue would have been unhandy). It prevents Windows users from seeing any setting at all (since they have no choice anyway) and Unix/Linux users from seeing the Windows-specific setting as well. The settings form has been moved to admin/settings/logging and the description has been simplified by moving part of it to the help page. Tests have been updated as well. Also, all occurences of the name 'Drupal' have been removed.

The form after the patch:

The help text before and after the patch:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

The last submitted patch failed testing.

Xano’s picture

I forgot to mention that. The patch was made outside of the Drupal root because of lack of CVS access. You will need to apply it using p1, IIRC.

Xano’s picture

Need to add OS X to the paragraph about UNIX/Linux.

Xano’s picture

Status: Needs work » Needs review
FileSize
8.11 KB

Follow-up patch. OS X is now mentioned as well.

keith.smith’s picture

Title: General cleanup and bugfix » syslog module: General cleanup and bugfix

IMO, if the individual operating system sections are to be separated out like this, they should not be as h2 level headings (where they will conflict with the help topic), but should probably be list items with a bolded section at the front describing the OS.

Xano’s picture

The headings are wrong in Garland, since the help topic should be a <h1>.

Status: Needs review » Needs work

The last submitted patch failed testing.

Xano’s picture

Status: Needs work » Needs review
FileSize
8.58 KB

This patch fixes a bug in system_watchdog() that caused a PHP error if the hook was invoked for the first time.

I suggest we change this as well:

function syslog_facility_list() {
  return array(
    LOG_LOCAL0 => t('LOG_LOCAL0 - Local 0'),
    LOG_LOCAL1 => t('LOG_LOCAL1 - Local 1'),
    LOG_LOCAL2 => t('LOG_LOCAL2 - Local 2'),
    LOG_LOCAL3 => t('LOG_LOCAL3 - Local 3'),
    LOG_LOCAL4 => t('LOG_LOCAL4 - Local 4'),
    LOG_LOCAL5 => t('LOG_LOCAL5 - Local 5'),
    LOG_LOCAL6 => t('LOG_LOCAL6 - Local 6'),
    LOG_LOCAL7 => t('LOG_LOCAL7 - Local 7'),
  );
}
function syslog_facility_list() {
  return array(
    LOG_LOCAL0 => 'LOG_LOCAL0',
    LOG_LOCAL1 => 'LOG_LOCAL1',
    LOG_LOCAL2 => 'LOG_LOCAL2',
    LOG_LOCAL3 => 'LOG_LOCAL3',
    LOG_LOCAL4 => 'LOG_LOCAL4',
    LOG_LOCAL5 => 'LOG_LOCAL5',
    LOG_LOCAL6 => 'LOG_LOCAL6',
    LOG_LOCAL7 => 'LOG_LOCAL7',
  );
}

The strings we have now don't add anything to the codes themselves.

Dries’s picture

This looks good to me but I don't have a MS Windows environment to test this on.

Status: Needs review » Needs work

The last submitted patch failed testing.

Xano’s picture

Status: Needs work » Needs review
FileSize
18.62 KB
8.07 KB

brianV’s picture

The Linux portions look good to me. However, I can't test the Windows portions.

Status: Needs review » Needs work

The last submitted patch failed testing.

Xano’s picture

Status: Needs work » Needs review
Issue tags: +Usability, +DrupalWTF
FileSize
8.31 KB

Tagging this WTF because Windows users should *not* be able to select Linux facilities.

Dries’s picture

Status: Needs review » Fixed

Committed to CVS HEAD. Thanks!

cburschka’s picture

Status: Fixed » Needs work

I'd open a separate issue, but this is titled "cleanup" so it fits.

The help text contains a link to http://www.rt.com/man/syslog.5.html, which is a 404. This should be replaced with a more durable man link.

Xano’s picture

moshe weitzman’s picture

Status: Needs work » Fixed

pls open new issue.

Status: Fixed » Closed (fixed)

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

Xano’s picture

Assigned: Xano » Unassigned