trait_exists() is needed on PHP 5.4.0+, but will break PHP 5.3.*

[donquixote]

This is odd..
In the "paranoid" flavours of PSR-0 (*), to check whether inclusion of a file did indeed load the class, we currently do

class_exists($class, FALSE) || interface_exists($class, FALSE) || trait_exists($class, FALSE)

To be safe across PHP versions, we would have to do this instead:

class_exists($class, FALSE) || interface_exists($class, FALSE) || (function_exists('trait_exists') && trait_exists($class, FALSE))

This is slowly eating away performance, every time we add to this check.

(*) Other class loaders (Symfony, Composer) don't do this check, and as a consequence, risk a "cannot redefine class" fatal error.
This is not a problem in practice most of the time, because most PSR-0 libraries don't use underscores in the class name, or they don't use class_exists() in a careless way.
Krautoload provides plugin that reproduce the unsafe behavior of other class loaders, and other plugins that act "safe", but at the price of slightly more expensive operations.

[donquixote]

I am going to add the suggested fix. Then we can still discuss alternatives.

[donquixote]

At the same time, maybe I should make the "risky" behavior the default one..
if it is good enough for Composer, then it is good enough for us. And if you run into problems, you can enable the safe mode.

[paul-vg]

I was actually surprised the PR got merged because of this issue. Was already working on a patch to fix it as well, by eliminating all the duplicated code and moving it to one place. Though I'm not 100% sure yet how to ensure the "if (php supports traits)" is only done once. Or if it even matters.

[paul-vg]

Commit 78c60a1 fixes this. Although I dislike duplicate code and I think it would be better if those checks sat in their own, shared method. @donquixote, should I propose a new PR which does that?

[donquixote]

I think a reasonable strategy would be to inline this stuff on cases of "competitive performance", and factor it everywhere else.
The Util::classIsDefined() is the reusable place where this is defined.
(imo this is exactly the kind of thing where a static method is justified)

Btw, there are interesting ways to do PHP version checks.

E.g. in src/Krautoload/Util.php, we could do this:

if (function_exists('trait_exists')) {
  require dirname(__FILE__) . '/Util.php54up.php';
}
else {
  require dirname(__FILE__) . '/Util.php53.php';
}

and then have version-specific classes code in those two files.

[donquixote]

should I propose a new PR which does that?

In some cases I feel feedback to be much more valuable than patches/pull requests.
On the other hand, merging a PR is a nice way to give attribution to people who are helpful.

One risky thing is I am still heavily shuffling around classes, and a simple and innocent space-at-line-end patch can easily result in a merge conflict.

There are other areas where I am absolutely happy about patches, esp stuff that I've never done before. E.g. the Travis stuff. I still need to figure out what that is :)

[paul-vg]

My idea is to either do something like this in Util:

function thing_exists($thing, $autoload = FALSE) {
  if (PHP_VERSION_ID >= 50400) {
    return class_exists($thing, $autoload)
      || interface_exists($thing, $autoload)
      || trait_exists($thing, $autoload);
  }
  return class_exists($thing, $autoload)
    || interface_exists($thing, $autoload);
}

This would not require any initialisation and leave it up to an optimiser to eliminate dead code.

XOR, create an include file:

namespace Krautoload;

if (!function_exists('trait_exists')) {
  function trait_exists() {
    return FALSE;
  }
}

And then include_once/require_once it from the relevant constructors. This would still not require any special initialisation and is similar to UpgradePHP but with a twist: the stub is local to the Krautoload namespace. So either the global PHP implementation will be used, or the Krautoload-local stub would be used transparently.

[donquixote]

"leave it up to an optimiser to eliminate dead code."

Which is not available everywhere, is it?

"the stub is local to the Krautoload namespace"

This sounds fun..
and it leads me to one thought. is \trait_exists() faster than just trait_exists(), when in a namespace? Or does PHP optimize that away by default?

[donquixote]

"PHP_VERSION_ID >= 50400"

I assume this is generally faster than function_exists('trait_exists'), is it?
(and easier to optimize)

So it would be a start to just replace the function_exists() by this version check.

[paul-vg]

That would be a start, although I'd always favour consolidating copy/paste code into a separate file.

And no, "\anything()" isn't faster inside or outside a namespace scope. People shouldn't be micro-optimising like that, anyway. That just results in a mess. In general, convey the intent rather than the implementation details. It allows the language developers to make more meaningful improvements. (If I had a penny for every kludge in PHP...)

But yes, comparing an integer is better than function_exists in a critical path. Because traits were introduced in 5.4.0, the check is still reliable.

[donquixote]

The main performance goal is to speed up average PSR-0 and PSR-X class loading.
Other tasks (discovery, loading exotic libraries, finding classes instead of loading them) are less critical.

There are exactly two places where inlining the triple check might be worth it:

• NamespacePathPlugin_ShallowPSR0::pluginLoadClass()
• PrefixPathPlugin_ShallowPEAR_Uncertain::pluginLoadClass()

I am going to leave these as inline, and replace all the rest with Util::classIsDefined().
Once we have benchmarks in place, we can discuss killing the remaining two.

[donquixote]

People shouldn't be micro-optimising like that, anyway.

I would normally agree. But classloader stuff is all about micro-optimization. E.g. if you compare APC cache vs classmap, then every additional indirection becomes measurable.

[donquixote]

Once we have benchmarks in place

I did some benchmarks on previous versions. But currently we have nothing working, afaik. New issue.