Load data containers statically

[leofeyer]

Replace $this->loadDataContainer() with Controller::loadDataContainer(). This will require to adjust any calls in the object context:

// Old
$this->loadLanguageFile();

// New
Sytstem::loadLanguageFile();

// Old
if ($this->User->isAdmin) { … }

// New
if (BackendUser::getInstance()->isAdmin) { … }

Otherwise the script will exit with a fatal "using $this when not in object context" error.

[xchs]

:thumbsup:

[iCodr8]

+1

[intco]

:thumbsup:

[aschempp]

I think this should be in Contao 3.1. I can't see a reason why loadDataContainer can't be static?

[leofeyer]

We could declare the method static of course, however you should not load DCAs statically right now. There are a lot of $this->loadLanguageFile() and $this->User->isAdmin calls in the DCA which will all fail if the DCA is loaded statically (using $this when not in object context).

[aschempp]

I see. Doesn't this affect loadLanguageFile too (which is static) ?

[leofeyer]

No, because language files only contain XML data (or PHP sttings).

[aschempp]

You have no idea how we hack the system. But anyway, I like the static method :D

[aschempp]

Hahaa!! I have finally found the solution! It should work similar to the DcaExtractor class. We need a class that loads the DCA, and it should work similar to the database class (multi-singleton).

[HellPat]

We do you make methods static? I don't really understand it.

[tristanlins]

@psren to break the horrible and overloaded class inheritance. When the methods are static, there is no need to extend from Controller or System to call this methods.

[tristanlins]

@psren in fact, there are better solutions like dependency injection, we decided to make the methods just static to keep the API break as small as possible.

[HellPat]

@tristanlins thanks. That was the answer i needed. What arguments speak against the facade-pattern powered by a dic? Is it the backwards compatibility? Would be a huge improvement in my opinion.

I guess this will be part of Contao Reloaded, right?

[tristanlins]

I guess this will be part of Contao Reloaded, right?

Yes, it is.

[aschempp]

Why reloaded? This could be added soon?

[leofeyer]

Can you post a code sample of your idea?

[HellPat]

A class is registered in the dic (we don't have that so far, am i right?) Dependencies will be injected via constructor injection here.

$this->app->bind('UserRepositoryInterface', function(){
  return new UserRepository(
    new UserRepositoryCache
  );
});

Now i can access the class with e.g. $this->app->make('UserRepositoryInterface'). An instance of UserRepositoryCache is injected.

A Facade is a simple class which maps static calls with __callStatic to the method of the class instance. E.g.: https://github.com/illuminate/support/blob/master/Facades/Facade.php

I can then use User::findById(3). This will get an instance of the UserRepository stored in the dic. and then call the method findById. => simple Usage

The main advantage ist, that the classes are still swappable.

Now i want my implementation of UserRepository so i do

$this->app->bind('UserRepositoryCacheInterface', function(){
    new UserRepositoryCache
});

$this->app->bind('UserRepositoryInterface', function($app){
  return new MyUserRepository(
    $app->make('UserRepositoryCacheInterface');
  );
});

and now my "Facade" uses this objects from the dic

UserRepository::findById(4);

As you see the implementations are swappable. E.g. a Mockery Mock could be injected and used while using the facades.

[HellPat]

Should be open a new Ticket for that? Ists a bit messy here because it's only releated to this topic.

:-) This would be very nice to be implemented. There are many DiC ready to use http://symfony.com/doc/current/book/service_container.html https://github.com/fabpot/Pimple https://github.com/illuminate/container/blob/master/Container.php (Pimple with some more features)

[aschempp]

I'm not talking about Dependecy Injection here ;-) I explicitly meant this for Contao 3.x ...

[leofeyer]

Me too, so please move the DI related stuff to a separate ticket and @aschempp please post a code sample of your idea here.

[HellPat]

Do i really have to open a Ticket for the DiC-Thing if it will not be implemented in Contao Reloaded?

[tristanlins]

@psren there is already an extension that provide a dependency injection container. The only think that is not supported yet, is the Facade pattern.

[aschempp]

Pretty simple:

class DcaLoader extends \Controller
{

    /**
     * Table name
     * @var string
     */
    protected $strTable;

    public function __construct($strTable)
    {
        $this->strTable = $strTable;
    }

    public function load()
    {
        // Here comes the code from Controller.php
    }
}

// in Controller.php
function loadDataContainer($strTable)
{
    $objDCA = new DcaLoader($strTable);
    $objDCA->load();
}

The only thing here is about the fact that you currently can't use loadDataContainer if you do not extends Controller. And we found out we can't use the method statically, because of lots of $this-usage in DCA files. But that can all be easily solved.

[leofeyer]

Indeed, this seems to work. We can then add this to Contao 3.3, since there is no BC break anymore. However, I'd prefer to have a DataContainer::load() method instead of Controller::loadDataContainer().

[aschempp]

We need to keep the Controller::loadDataContainer for backwards compatibility reason. And do you think it would be right to add a static method that does nothing else than create an object and call the method? I don't think so. The DcaExtractor does not offer that either. Maybe this should be unified anyway.

[leofeyer]

And do you think it would be right to add a static method that does nothing else than create an object and call the method?

This is not a question of right or wrong. You surely could ask what's the point, but @tristanlins explained it above: decoupling. You can then load a data container without having to extend the Controller class.

[aschempp]

Uhm… You can't have a static method in DataContainer to do this? Static method in DataContainer would have the same problem as static method in Controller? So we still need the DcaLoader class?

[leofeyer]

Yes.

class DataContainer
{
    public static function load($strTable, $blnNoCache=false)
    {
        $loader = new DcaLoader($strTable);
        $loader->load($blnNoCache);
    }
}

class Controller
{
    /**
     * @deprecated Use DataContainer::load() instead
     */
    protected function loadDataContainer($strTable, $blnNoCache=false)
    {
        DataContainer::load($strTable, $blnNoCache);
    }
}

[aschempp]

I don't like the idea of a static method that does only initialize an object. How about this?

class DcaLoader extends \Controller
{

    /**
     * Table name
     * @var string
     */
    protected $strTable;

    /**
     * DCA cache
     */
    protected $arrCache = array();

    public function __construct($strTable)
    {
        $this->strTable = $strTable;
    }

    public function load()
    {
        // Here comes the code from Controller.php
    }

    public static function forTable($strTable, $blnNoCache=false)
    {
        if ($blnNoCache || !isset(static::$arrCache[$arrTable])) {
            static::$arrCache[$arrTable] = new static($strTable);
        }

        return static::$arrCache[$arrTable];
    }
}

I can then use this shortcut in my code:

DcaLoader::forTable('tl_content')->load();

Where load() obviously checks if it is already loaded, and because it is an object instance we can store the info in the object. And because the object exists only once for each table (using the cache), stuff like getRelations() will probably be much faster if used multiple times.

[leofeyer]

This code is untested, right? You are using $this in a static method, PHP would have thrown an error if you had tested it :)

[aschempp]

Yes it's untested. Fixed my code ;-)

[leofeyer]

Implemented in 02f633c4a3658e2490e4af6f6dab40698b45fa79.

[aschempp]

I don't think we should use the DataContainer class as a helper, because that's the Driver base class. Why not simply do something like DcaLoader::loadStatic (bad name, but you get the point)?

[leofeyer]

I've read your concerns above already, but I don't see the problem. The DataContainer class is the mother of all data container classes, so it feels quite natural to add the load() method there.

[backbone87]

but you dont load the DataContainer, you load the DataContainerArray.

DataContainer is, like aschempp said, the driver class.

If you would be true object oriented, you would do something like this:

$dca = new DataContainerArray('tl_module');
$dca->load();

or

$dcaf = new DCAFactory;
$dcaf->load('tl_module');

[leofeyer]

That's splitting hairs, don't you think? The previous method was called Controller::loadDataContainer() and nobody ever mentioned it should be Controller::loadDataContainerArray() instead.

[leofeyer]

If you would be true object oriented, you would do something like this:

We are doing exactly this:

$dca = new DcaLoader('tl_module');
$dca->load();

The static method is just a convenience method.

[aschempp]

Ok, then I'd say we should remove the convenience method. There isn't any for DcaExtractor either. And if we make Controller::loadDataContainer static, we've solved all problems ;-)

[leofeyer]

Agreed and changed in 3d93455e23133581a86314a15fa3508c87244b18.