Redundant check in Autoloader.php

[tsahi-gisha]

The following code is taken from Autoloader.php:

    /**
     * Handles auto loading of classes.
     *
     * @param string $class A class name.
     */
    public static function autoload($class)
    {
        if ($class[ 0 ] !== 'S' && strpos($class, 'Smarty') !== 0) {
            return;
        }
        $_class = strtolower($class);
        if (isset(self::$rootClasses[ $_class ])) {
            $file = self::$SMARTY_DIR . self::$rootClasses[ $_class ];
            if (is_file($file)) {
                include $file;
            }
        } else {
            $file = self::$SMARTY_SYSPLUGINS_DIR . $_class . '.php';
            if (is_file($file)) {
                include $file;
            }
        }
        return;
    }

this line does a redundant check:

        if ($class[ 0 ] !== 'S' && strpos($class, 'Smarty') !== 0) {

The second condition is always true.

[jens1o]

No, it's not when $class is something like SuperWoman?

[tsahi-gisha]

@jens1o in this case the second condition is never reached.

[jens1o]

Yeah. The first condition is only there to avoid calling the rather expensive strpos function. (=> Performance Improvement)

[uwetews]

Depending on your use case several autoloader can be active and will be called until the handler which can handle the reqested class can be found. Any autoloader which can not handle a request should exit as fast as possible. The first condition is very quick and does sort out all requests not starting with 'S', the second is more time consuming and does sort out anything not starting with 'Smarty'. Note that strpos() is faster as any string compare functions.

[tsahi-gisha]

@jens1o @uwetews

Believe me that I understand the concept of having the first condition resolve as fast as possible.

But the logic of this statement is flawed.

It should probably be changed to:

if ($class[ 0 ] !== 'S' || strpos($class, 'Smarty') !== 0) {

But at its current form (using &&), the second condition is completely useless. Either it's never reached, or it's always true.

If you can't understand basic boolean logic, then I give up.
I demonstrated good citizenship by reporting this.

[artcs]

tsahi-gisha is correct of course. The && statement is logically wrong and should be changed to || for classes like SuperWoman to return early.

[uwetews]

This fix is now in the master branch

[matks]

Hi guys,

Today at PrestaShop we found that the flawed if ($class[ 0 ] !== 'S' && strpos($class, 'Smarty') !== 0) statement can be very costly for people using autoloaded classes with an "S" as 1st letter... like Symfony framework

In our case, since Smarty autoloader was called before the Composer autoload, every Symfony class (like Symfony\Component\Config\ConfigCache for example) was going through this if and skip the return;, this consequently would trigger the is_file some lines later. On a in a virtualized environment using NFS, this means a huge number of bad I/O which results in big latency or even crashes.

So first of all thanks for fixing this 😉

We're going to fork Smarty and use our fork patched version until the next release. Do you have planned the next release ? If this has impacted us, there might more people impacted by this.