Silence include warnings in rcube_autoload

[cabeca]

Hi,

In function rcube_autoload, the last step is including the filename derived from the classname with an include statement:

include $filename. '.php';

This creates a problem for classes defined in libraries with their own autoload functions registered with php's spl_autoload_register. These libraries could be in use in plugins, for example.

spl_autoload_register creates a queue of functions to be called in order of registration, when a class is referenced.

rcube_autoload should ignore classnames that it doesn't know, and let following registered autoload functions do their job.

Trying to include filenames derived from classnames that roundcube doesn't know creates PHP warnings for every foreign classname.

A simple fix is prepending an @ sigh before the include statement to silence it's warnings:

@include $filename. '.php';

A more complete fix should list every classname used by roundcube, and ignore autoload requests for classes outside this list.

Best Regards
Miguel Cabeça

[vminakov]

@ - is not the best solution. (​http://php.net/manual/en/language.operators.errorcontrol.php and various comments below)
Probably, as temporary fix just check if file exists.

However, I guess a proper autoloader component with an ability to register own autoloaders for any specific needs (like it's done in ZF) would be a more flexible and sophisticated solution

[cabeca]

Checking for file existence would be easy if roundcube didn't use any library in include_path, like PEAR and MDB2. Checking for those would be tricky.

IMHO the roundcube autoloader should only try to do something with classnames that it knows about, leaving unknown classes to be handled by the next autoload function registered with spl_autoload_register. Don't get me wrong, but trying to reimplement this functionality (ability to register own autoloaders for any specific needs) on a more complete roundcube autoloader function seems like a waste of energy.

Miguel Cabeça

[cabeca]

Hi,

I had a look at how MDB2 deals with this issue and taking the hint of vminakov, I've made this patch that checks if the file exists before including it.
I've also changed include to require_once, that will force a failure if the file does exist, but the loading failed. Is this a desired feature, or should it remain an include instead of require_once?

Best regards
Miguel Cabeça

[vminakov]

It looks nice, I agree. Let's wait for RC developers respond to it.

Btw, do we really need require_once or include_once? If roundcube core (and plugins?) rely only on autoloader, changing require_once to require would give a slight performance improvement. Sadly, I can't test it right now and check whether it would throw something like 'cannot redeclare class'

On the other hand, since Roundcube works only on PHP >5.2, it's possible to tune require_once performance by adjusting ​realpatch_cache_size and realpath_cache_ttl. Roundcube doesn't have very many php classes and a complex directory structure, so probability of realpath cache misses is likely to be low.

[thomasb]

Fixed in [965ed0ca]

[brandond]

A full fopen/fclose seems awfully heavy-handed; couldn't you just use is_readable?

[ekrieger]

​http://www.php.net/manual/de/function.is-file.php

[thomasb]

fopen does filesystem operations (I know, file handles are assigned, etc.) and should be processed at reasonable speed by the operating system. I successfully used this mechanism on several large scale websites without significant loss of performance. I think fopen is faster than manually walk the include_path as proposed in the attached patch.

[alec]

I've made a test. is_file+is_readable is 10x faster than fopen+fclose.

[alec]

NO, I'm wrong. It's 5x faster, but because it doesn't search include_path in our case fopen+fclose will be probably better.