System::import() may return unexpected instances

[dmolineus]

Affected version(s)

• ^4.9.4

Description

Since #1840 a System#import() or System::importStatic() may return an unexpected instance. This is the case for the User import in Contao. How to reproduce it:

class CallbackA extends \Contao\System
{
    public function __invoke()
    {
        $this->import('Frontenduser', 'User');
        var_dump(get_class($this->User));
    }
}

class CallbackB extends \Contao\System
{
    public function __invoke()
    {
        $this->import('BackendUser', 'User');
        var_dump(get_class($this->User));
    }
}

(new CallbackA())();
(new CallbackB())();

If a third party extension imports the backend user before the frontend user is imported (e.g. through a hook), the frontend user is not used in all legacy classes. Which might lead to an invalid upload folder path in the FormFileUpload form field.

[aschempp]

I understand the problem, but I would assume it does not happen with System::importStatic() but only System::import(). Can you confirm that?

[dmolineus]

I understand the problem, but I would assume it does not happen with System::importStatic() but only System::import(). Can you confirm that?

It also happens when System::importStatic() is used. It also accesses the static cache, see https://github.com/contao/contao/pull/1840/files#diff-29552df618c525c0eb4fd5126fbe4e6fR234

var_dump(get_class(\Contao\System::importStatic(\Contao\FrontendUser::class, 'Foo')));
var_dump(get_class(\Contao\System::importStatic(\Contao\BackendUser::class, 'Foo')));

[aschempp]

yes, but isn't that how importStatic() always worked? The "services" are shared across class instances?

[leofeyer]

I think we can fix this by using both $strClass and $strKey as cache key. Then $this->import('FrontendUser', 'User') will be stored separately from $this->import('BackendUser', 'User'). @Toflar WDYT?

[ausi]

This happens only with classes that have a getInstance() method, right?

I think we can fix this by using both $strClass and $strKey as cache key.

I think the correct fix would be to use static::$arrSingletons[$strClass] instead of static::$arrSingletons[$strKey] and keep using $this->arrObjects[$strKey]. So using the class name for the static cache and the instance name for storing the object in the local array.