0

I have what is probably an over-engineering question. I have some PHP code that logs messages to a file. We've been having a problem with the file having the wrong perms from time to time. While we're troubleshooting that, I am writing a work-around for that in the logging function, changing the unix perms and owners on the log file.

So my code looks like this:

function log($message) {
  if ( FALSE === file_put_contents(log_path(), $message) ) {
    chown(log_path(), 'apache');
    chgrp(log_path(), 'www-data');
    chown(log_path(), 0770);

    // now I'm repeating this line
    if ( FALSE === file_put_contents(log_path(), $message) ) {
      // log error to database
      db_log($message);
    }
  }
}

In the interest of DRY, can I somehow change the code to avoid repeating the line if ( FALSE === file_put_contents(log_path(), $message) ) {? Of course, I can't use recursion, because that would go on forever.

|improve this question
  • If the second if condition requires the three ch lines in order to work properly, then your code is already DRY. – Robert Harvey Sep 9 '15 at 19:01
  • why would recursion go on forever? You're not doing recursion right if it's endless. – Jimmy Hoffa Sep 9 '15 at 19:09
  • @JimmyHoffa Well, if I did a simple log($message) within the function, it would continuously try to log, then fail, log, then fail, etc. I could put a static variable, or add an argument as a termination condition, but I never want it to recurse more than once anyway. Have I saved anything, then, over just repeating the line? I suppose again I come back to the thought that I am probably over-thinking things. – user1936 Sep 9 '15 at 19:22
  • 1
    Definitely overthinking, but hey, just for grins I gave you a solution anyway just as an exercise. – Jimmy Hoffa Sep 9 '15 at 20:13
  • 1
    Logging is hard, let's go shopping. function log($message) { syslog(LOG_INFO, $message); } is a far better answer, and it puts the hard part in rsyslog et al., where it belongs. – Ross Patterson Sep 10 '15 at 2:40
-1

It's unnecessary, but if you want to dry this up:

function and_($now, $then) {
    return function($seed) use($now, $then) {
        return $now($seed) && $then($seed);
    };
}

function or_($now, $else) {
    return function($seed) use($now, $else) {
        return $now($seed) || $else($seed);
    };
}

function write_message($message) {
  return file_put_contents(log_path(), $message);
}

function fix_file($message) {
  chown(log_path(), 'apache');
  chgrp(log_path(), 'www-data');
  chown(log_path(), 0770);
  return TRUE;
}

function log_($message) {
    $log = or_('write_message', and_('fix_file', or_('write_message', 'db_log')));
    $log($message);
}
|improve this answer
  • I find it strangely concerning that while I can follow the purpose of this quite easily on the first reading, I had to read it about 5 times to figure out how it does it. – Jules Sep 11 '15 at 13:44
  • @Jules the fact that it's easy to see what it does is the point - this is how this style of coding enhances maintainability. You can quickly and easily reason about what it's doing, but how it's doing it may be a little more involved. Since code is read wayy more often than it's written, this is a good thing. You'll spend a bit of time reading and re-reading the code you're going to change in maintenance anyways, so that it takes some effort to understand the how is not really a problem in my experience. You'll rarely edit such code though, you'll read it and know what it does and move on. – Jimmy Hoffa Sep 11 '15 at 14:11
  • Please don't ever do this. – Corbin March Oct 30 '15 at 20:10
  • @CorbinMarch to be fair- that's my rule of thumb for people not used to the style. When you're familiar with the style, it's just as easy to edit but for people unfamiliar just see english that's easy to understand but the implementation uses a variety of unfamiliar abstraction techniques regarding functional composition. When you have seen it enough times to be familiar with such techniques; they're very easy to modify. – Jimmy Hoffa Oct 30 '15 at 22:54
1

If you absolutely wanted to, you could do this:

function log($message)
{
  for($i = 0; $i < 2; ++$i)
  {
    if ( FALSE !== file_put_contents(log_path(), $message) )
    {
      return;
    }
    if($i == 0)
    {
      chown(log_path(), 'apache');
      chgrp(log_path(), 'www-data');
      chown(log_path(), 0770);
    }
    else
    {
      // log error to database
      db_log($message);
    }
  }
}

I don't like it, however... Just a simple function call? Well, it is OK. If it was a more complex code you had to execute before being able to make a decision, I'd write a separate function for this, returning true or false, and have it called twice, right as you did with the file_put_contents function.

|improve this answer
0

A couple possibilities that haven't been mentioned:

  1. If setting the permissions is always acceptable, always set the permissions, avoiding the first 'if' entirely. Especially since this is a workaround.
  2. Check the permissions, setting them if necessary, then call file_put_contents(...). This has the added benefit of distinguishing a permissions error from a real error in the call to file_put_contents(...), which may help with debugging.
|improve this answer

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.