@dongilbert This is your child. How bad is it screwed? (saying that without having looked yet)

Well I think it's "not THAT bad".... that's why I'm putting it on the table

The CLI app is running fine. I suspect it's something around the session (?) Or who else would tries to serialize objects ?

Oh and --- I think I only adopted this child (doublesmiley)

I'll have to take a look maybe on lunch today when I have more time.

I should note, however, that we're starting to use the Container more like a Service Locator instead of as a tool to inject dependencies. Like here https://github.com/joomla/jissues/pull/167/files#diff-76025bb43adcb9a625cba7c97cc67c34R159, we should pass in the db as a constructor parameter, instead of just passing in the container and then getting the db from the container. Doing it this way makes it clear the object depends on the container, instead of hiding that dependency behind a service locator, know what I mean?

The idea for proper usage of the container would be to register all your classes dependencies via ServiceProviders or some other app bootup, and then use the container to build your domain objects. This keeps the container out of the business / domain layer, while increasing testability.

TL;DR The container should really only be available in the application / controller classes of our app. There are some instance where it's useful otherwise, but those are few.

That said, this is an improvement over using the static Container:: methods to get the objects. At least the code is now apparent that it depends on the container.

Thanks Don, that's quite enlightening ;)

The first file you mention - is actually a special case - the CliApp\Command\* classes are something like "mini applications" - it's quite handy to have the container here ;)

I also added a Container to the Table __constructor()s instead of $db - I think that's what you meant, so I reverted it.

Speaking of controllers: What if we not implement the ControllerInterface nor the AbstractController in our AbstractTrackerController and instead just pass the Container to the __constructor() ?

I tracked down the error about the serialization of the closure to the session - "disabling" the session works just fine..

If you want the controller in your container, I would implement the ContainerAwareInterface and then check for that where you're instantiating your controllers. If you have a container aware controller, then pass in the container using the setContainer method. Does that help?

—
Sent from Mailbox for iPhone

On Mon, Nov 4, 2013 at 1:08 PM, Nikolai Plath notifications@github.com
wrote:

Thanks Don, that's quite enlightening ;)
The first file you mention - is actually a special case - the CliApp\Command\* classes are something like "mini applications" - it's quite handy to have the container here ;)
I also added a Container to the Table __constructor()s instead of $db - I think that's what you meant, so I reverted it.
Speaking of controllers: What if we not implement the ControllerInterface nor the AbstractController in our AbstractTrackerController and instead just pass the Container to the __constructor() ?

I tracked down the error about the serialization of the closure to the session - "disabling" the session works just fine..

Reply to this email directly or view it on GitHub:
#167 (comment)

Sorry, the opener should have read "if you want the container in your controller"

Ah OK, that makes sense then - sounds like very interesting ;)

@elkuku I think this may be a good example on how to pass container to controller:
https://github.com/eddieajau/joomla-rest-server/blob/master/src/Server/Application.php#L136

I think the thing that bugs me is having to implement methods like this one in my base classes, so it's more like a hidden "soft" dependency.
Should we use the same technique in models and views ?
Thanks for sharing your thoughts

If you follow the heart of what is in the DI docs then you should only need the container (and the interface and its methods) in the app and controller classes as the controller should be building the MV part of the triad and injecting dependencies. With that said, I can't say I know with absolute certainty what right looks right for us, but that should help us move in the right direction.

Think I only read the first draft of the docu... thx for the link, I'm about to understand how Joomla! is supposed to work now ;)

That's understandable, it was just recently updated. Have fun!

I'm not still completely sure though what goes into ServiceProviders and what stays as an Application method.

DIC should be accessible (only) by Controllers but these have access to Application too, for example:

public function execute()
{
    $config = $this->getContainer()
        ->get('config');
    // Vs
    $config = $this->getApplication()
        ->getConfiguration();
}

IMO, items that are dependent on the app should be in the app and everything else be a service. Config isn't app dependent, I'd make it a service. We still have some stuff in our app that needs to be converted (we built a lot of things into the app needlessly simply to avoid using the old Factory and not having DI in place).

I'd put into DIC anything that's not in the AbstractApplication. But config is.

There is our TrackerPagination thingy that needs the current URI to build the links. The pagination is instantiated from our AbstractTrackerListModel providing the total and start count.
Currently the pagination uses AbstractWebApplication::detectRequestUri() to get that information.

So - How do we pass the application object around ?

There are also other instances where models and views need some information provided by application methods.

Thoughts ?

BTW: The above mentioned Exception: Serialization of 'Closure' is not allowed in [no active file] on line 0 has been fixed by providing the objects with their proper serialize() methods.

There is still a lot of thinking required before considering a merge. If you see any more "NONOs" - please point them out

I have a problem with TrackerPagination as much as I have with JPagination in the CMS.
IMO it's wrong that it reads current page from Uri:

• you can't use it in CLI
• it prevents from using multiple paginated List views in one page
• It mixes business logic with HTML output so you are forced to use Bootstrap 2.

Unfortunately I don't have a complete alternative yet.
ATM I'm using simple object that I configure in controller based on Model results and pass to View.

FYI The last one will be solved in JPagination in the CMS as I'm going to make it use JLayout's when I get around to it (but I've been saying this for the last 3/4 months so not sure when)

@piotr-cz that's the problem - no alternatives :( But may be we can use some JS-based pagination like this or this ?

@piotr-cz Yes exactly that's what's bothering me... and TBH I haven't really been thinking about a CLI pagination yet...
That said, the TrackerPagination is really just a first cut, which isn't even my code (docs)... and I think there is a lot of room for improvements (complete or incomplete) ;)
I think you are completely right about the pagination not being responsible of rendering HTML, nor it should know anything about an URI object...
Please improve :)

I agree about URI, but for example, how to get to the 3rd page of the list using a direct link? Without query param it is not possible, right? :)

@b2z right, but in my opinion pagination shouldn't be read in by TrackerPagination, but by Controller or maybe View. I'm not sure where/ how.

I'm not comfortable with the concept of models populating the state:

Application passes Container to Controller, Controller to View, View to Model and that one finally reads $application state and $input.

I'd much rather see controllers doing it.

@piotr-cz do you mean something like this - controller is responsible for various filters, sorting and pagination params?

BTW I have applied JS pagination (Bootstrap Paginator) and you can see it in action on my demo site. If we will use this then our TrackerPagination class can be boiled down to only two methods: getPageNo() and getPagesTotal(). And we still can use GET to retrieve pages. The only problem is that when the page is loading a pagination placeholder is empty (try to F5 for several times) :( Dunno how to solve this :(

@elkuku I think the DIC is supposed to be used this way:

For a model with dependencies defined in constructor

MyModel extends AbstractDatabaseModel
{
    public function __construct(DatabaseDriver $db, Registry $state = null)
    {
    }
}

you create new model in a Controller using container's buildObject method:

//  $model = new $mClass; // would need to inspect the instance and pass dependencies
    $model = $this->container->buildObject($mClass); // DIC managed dependecies

DIC reads model's dependency as fully qualified class name Joomla\Database\DatabaseDriver\DatabaseDriver, looks it up in registered services and use when instantiating new object.

The DIC documentation has been recently updated, let's confirm my theory with @dongilbert

This is quite easy for this case, but might be a challenge for other parts of code.