Reading the code in nagios.module on line 240:

<?php
// Print the output, so we test the theme
print theme('page', $output);

// Exit early so we do not cache the data, nor do we wrap the result in a theme
exit();
?>

Reading the comment right before exit(); and then looking at the print theme('page', $output); line this seems to be contradictory. Should this:

// Print the output
print $output;

// Exit early so we do not cache the data, nor do we wrap the result in a theme
exit();

not be more than sufficient? We really do not need this page to have any look and feel, no?

CommentFileSizeAuthor
#6 nagios-nagios_page_styling-1162586-6.patch1.15 KBvictoriachan
#5 nagios-nagios_page_styling-1162586-5.patch842 bytesvictoriachan

Comments

greg.harvey’s picture

greg.harvey
Primary language English
Location Occitanie, France
commented
Version: 6.x-1.0-beta1 » 7.x-1.x-dev
Status: Active » Needs review

Sounds reasonable. To be honest, I never use the actual page cos we use the drush commands with NRPE, but hitting the page in a browser I think you're right.

So, here's the big one? Will changing that cause problems for existing installations? I guess not, but I'd like to be sure of that prior to just changing it. I'm still not hugely familiar with the code myself, having focused on the drush side, so I'll need to sit down and acquaint myself better.

greg.harvey’s picture

greg.harvey
Primary language English
Location Occitanie, France
commented
Version: 7.x-1.x-dev » 6.x-1.x-dev
Status: Needs review » Patch (to be ported)

Well, I didn't realise but raystuart already fixed this in his patch for Drupal 7, so a version of your code is already there in 7.x-1.x-dev ... code there reads:

  // Print the output, so we test the theme
  // TODO Please change this theme call as discussed at drupal.org/node/224333#theme_page.
  // print theme('page', $output);
  return $output;

  // Exit early so we do not cache the data, nor do we wrap the result in a theme
  exit();

So, if that doesn't cause any complaints after a while I see no problem with back-porting the change. We'll just let the D7 version sit for a while first. And of course the TODO is not relevant for D6.

malc0mn’s picture

malc0mn commented

Okay!

greg.harvey’s picture

greg.harvey
Primary language English
Location Occitanie, France
commented
Status: Patch (to be ported) » Closed (won't fix)

This is in the Drupal 7 branch already. Does anyone actually care about this in Drupal 6? I might just drop it. Frankly, it's dangerous to do this without someone currently using the HTTP check in Drupal 6 trying the patch and reporting if it causes them problems.

I'm going to "won't fix" it for now. If a few people follow up on this and report it doesn't cause Nagios any problems for existing implementations on update then I'll commit it, but I doubt anyone ever will. ;-)

victoriachan’s picture

victoriachan commented
Status: Closed (won't fix) » Needs review
StatusFileSize
new nagios-nagios_page_styling-1162586-5.patch842 bytes

Hi,

I've created a D6 patch according to the request on https://drupal.org/node/1337954 to make this use the theme function. By default it will output the nagios page as plain text. But for people who wish to output this with full page theme, they can still do so by overriding the theme.

Thanks,
Victoria

victoriachan’s picture

victoriachan commented
StatusFileSize
new nagios-nagios_page_styling-1162586-6.patch1.15 KB

Oops very sorry, the previous patch didn't include the new template file. Here's a new patch.

greg.harvey’s picture

greg.harvey
Primary language English
Location Occitanie, France
commented
Issue summary: View changes
Status: Needs review » Closed (won't fix)

Hi,

Actually, we took another direction in the other issue you referenced and removed all theming from the standard status page in favour of having the ability to alter the callback for the status page so people can provide their own: #1162332: Customise default /nagios callback. If you re-open that with a patch for Drupal 6, I'll happily commit it. :-)