21

I was building an exchange with a partners XML service, and I couldn't get the XML right, but as with all things Drupal, the xmlrpc error and action logging is less than robust.

So I did this in includes/xmlrpc.inc.

function xmlrpc_request($method, $args) {
  $xmlrpc_request = new stdClass();
  $xmlrpc_request->method = $method;
  $xmlrpc_request->args = $args;
  $xmlrpc_request->xml = <<<EOD
<?xml version="1.0"?>
<methodCall>
<methodName>{$xmlrpc_request->method}</methodName>
<params>
EOD;
  foreach ($xmlrpc_request->args as $arg) {
    $xmlrpc_request->xml .= '<param><value>';
    $v = xmlrpc_value($arg);
    $xmlrpc_request->xml .= xmlrpc_value_get_xml($v);
    $xmlrpc_request->xml .= "</value></param>\n";
  }
  $xmlrpc_request->xml .= '</params></methodCall>';

  /* This part here */
  watchdog('xmlrpc',$xmlrpc_request->xml);
  /* End ridiculously tiny hack */

  return $xmlrpc_request;
}

I got the data I needed and in 10 minutes had the partner interface responding appropriately to my request because (shocking I know) logs are good.

I like the extra logging, and I want to keep it. What is the clean, straightforward, and most importantly, Drupal-approved way of doing that?

  • 2
    I don't see why this was downvoted. Yes, editing core is discouranged but @OhkaBaka acknowledged this is is asking for suggestions on how to manage this and provided a real-world example. Along with the need to debug situations, there are legitimate reasons for editing core. There are some bugs in core w/ working patches in the issue queue that just won't get applied, and there are a few things that don't really have workarounds. – mpdonadio Mar 9 '12 at 17:15
  • 1
    The answers below are great. One thing I will add, though, is that you don't need the logging turned on all the time on your live site. Disable your custom module when you are not using it, or apply your patch or module to your dev site only. Minimizing changes and documenting carefully will keep you sane. – greg_1_anderson Mar 9 '12 at 18:01
  • @greg_1_anderson - You'll find that my solution below already addresses this through the use of a log_level variable (though using constants would obviously be cleaner, I omitted them to place emphasis on the pattern). This way you can use the same wrapper method on dev/live and the rest of your code can depend on it without changing function calls around. All you need is to set the logging level of your module using variable_set() or a similar mechanism that can be exported if need be. :] – David Watson Mar 9 '12 at 19:06
  • 1
    @David: Yes, that is awesome. I like to keep dev modules disabled on live, and enable them in a post-sql-sync hook, per drupalcode.org/project/drush.git/blob/HEAD:/examples/… Your technique gets top marks too, although I think I would also do the variable_set in a drush post-sync hook rather than a feature. Applying a patch on the dev system only, as I said above, is probably a bad idea unless you're sure the system is really a scratch system; otherwise the match might be accidentally committed and pushed. Ouch. – greg_1_anderson Mar 9 '12 at 22:54
  • @greg_1_anderson - I've actually been meaning to look into whether such hooks exist, and didn't realize there were already examples in place; many thanks for the link! Knowing now that this is possible, I agree that setting this at the environment level is definitely the way to go, both for the reasons you suggest and to help keep generic site configuration decoupled from environment-specific configuration. – David Watson Mar 10 '12 at 22:10
11

Hacking core is strongly discouraged for the uninitiated because it effectively reduces the support community of thousands to a support community of one (or whatever your team size is). Without this best practice, helping those new to Drupal would be nigh impossible. It also hinders modularity and, in some cases, security.

This having been said, hacking core is not always as evil as we like to make it out to be. Without modifying core, we wouldn't have distributions like Pressflow and the like that augment core in interesting ways. It's just vitally important that you know exactly what you're doing, that you're distributing your patches with your distribution (preferably in a way that allows you to apply them again automatically post-upgrade), and that you're keeping detailed documentation of what you've changed and why you've changed it.

Depending on how you have things structured, you could certainly make the above change to xmlrpc_request(), create a patch, and then use something like Drush Make to automate applying it (note that Drush Make is moving into the Drush project itself for the 5.x release), while supplying additional documentation in the makefile and elsewhere as to what the change does and why it's necessary/desired.

Another common pattern to enhance core functions is to create a wrapper that adds a tiny bit of functionality to a core function, and call the wrapper in lieu of core's implementation. When feasible, this makes things much more modular. Consider the following:

/**
 * Wrapper function for xmlrpc_request() to provide logging.
 */
function mymodule_xmlrpc_request($method, $args) {
  $xrr = xmlrpc_request($method, $args);
  watchdog('xmlrpc', $xrr->xml);
  return $xrr;
}

Again, depending on what you're doing this may or may not be feasible, but when it is you've saved yourself a few headaches in trying to make sure that core stays patched and documented. Though in this case, a one-off function like this seems like a perfect candidate for such a wrapper. If your implementation is captured in a module, you could even expand on it to control the log level of your entire solution, disabling this functionality on production sites:

/**
 * Wrapper function for xmlrpc_request() to provide logging (if enabled).
 */
function mymodule_xmlrpc_request($method, $args) {
  $xrr = xmlrpc_request($method, $args);
  if (variable_get('mymodule_log_level', 0) > 0) {
    watchdog('xmlrpc', $xrr->xml);
  }
}

In short, you want to maximize what you can do with modules (and you can do a lot), but there are legitimate reasons for altering core. It should be done with care, that's all.

9

If you need to alter core or contrib modules you should.

  1. Create a patch with changes.
  2. Use a deploy system like drush make that will automatically re-apply patches when you update core or modules.
  3. Document document document.
  • 1
    I genuinely wasn't expecting a validation of altering core by any stretch of the imagination... so I'm now forced to move to a secondary question: Is this in any way better than doing the same thing in a stand alone module? – OhkaBaka Mar 15 '12 at 20:02

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.