replace is_file() + require_once() with include_once() for apc performance

Can we flag this for a quick backport to D7, once all tests are passed? I could really use this in a productive system.

Also, here is another quick fun fact about include_once(), in case someone sees use for it:
The first time, when a file is actually included, it returns int(1). When called a second time it returns bool(true).

I did not know that and it does sound like a problem...

Is the @ even necessary in the first place? How typical is it for module_load_include() to be called on a non-existing file? If it should never happen, maybe having the php warning thrown is in fact the desired behavior.

Ok, maybe I can restart the conversation here...

I would still argue that a warning makes sense in case module_load_include() is invoked on a non-existing file.

If that is not the case and these errors should be ignored silently, then we should intercept the warning and react to it. Since Drupal already registers a custom error handler during bootstrap, this could happen through a modification of _drupal_error_handler_real()...

class IncludeException extends ErrorException {}

function _drupal_error_handler_real($error_level, $message, $filename, $line, $context) {
    //...
    if ($error_level == 2 && substr($message, 0, 7) == 'include'){
        throw new IncludeException($message, 0, $error_level, $filename, $line);
    }
    //...
}

Then module_load_include() could catch this custom exception class. Of course this would also possibly impact the behaviour of module code if they use include() directly.

Another alternative, without modification of the Drupal error handler, would be to use a temporary override:

function module_load_include($type, $module, $name = NULL) {
  if (!isset($name)) {
    $name = $module;
  }

  if (function_exists('drupal_get_path')) {
    $file = DRUPAL_ROOT . '/' . drupal_get_path('module', $module) . "/$name.$type";
    $file_exists = true;
    
    //PHP 5.3 closure makes this easier, but could be written backward compatible
    set_error_handler(function ($error_level, $message, $filename, $line, $context) use (&$file_exists){
        if ($error_level == 2 && substr($message, 0, 12) == 'include_once'){
            $file_exists = false;
            return true;
        }
        return _drupal_error_handler_real($error_level, $message, $filename, $line, $context);
    });
    
    include_once $file;
    
    restore_error_handler();
    
    if ($file_exists) {
      return $file;
    }
  }
  return FALSE;
}

Try/Catch only works for exceptions, not errors. These are separate concepts in PHP (although unifying them is being discussed). Errors can only be intercepted through set_error_handler().

include() triggers an E_WARNING type error, while require() produces an E_COMPILE_ERROR which is fatal by definition and ends script execution immediately.

Like I said, Drupal already registers an error handler which reroutes all warnings to the dblog. So the worst outcome of using include_once() without @ would be that more warnings would end up in the log, whenever a module tries to include a non-existing file.

As long as we still evaluate the return value of include_once(), the return behaviour of module_load_include() should remain exactly the same.

Just did a little test with some Drupal installations that have a lot of third party modules installed...

Sadly, it looks like it is not a rare event that modules try to include non-existent files.

So, yeah, that basically leaves me with my solutions proposed in #10.

Everything in #10 is happening within PHP, so the impact should be negligible.

You are probably right though, that on a regular installation within a local file system, the performance gain will also be minimal, if the underlying operating system has the requested file system entries cached.

The real reason why I opened this issue in the first place was to avoid file system checks in case of a remote shared file system installation, because these are very costly.

PHP 5.5 now includes the new Zend OPcache by default, while APC is being discontinued. It's also available for earlier versions of PHP.

There is now a config option "opcache.enable_file_override", which makes functions such as is_file() skip the file system access if the requested file is a script which has been cached.

Since this is a suitable solution for the future and a legacy workaround within Drupal seems to be too complicated, I am now closing the issue. Thank you all for your input anyway!