[5.0] Improve constructors of JTable objects

[wilsonge]

Summary of Changes

This standardises the fact the dispatcher can be injected into any JTable object. Additionally it phases out the use of \Joomla\CMS\Table\Table::getInstance in favour of directly instantiating the class. This is only for Table classes in the same extension or that are in the library folder. The correct way to do cross-extension table instantiation remains through the MVCFactory.

Removing using getInstance is important to allow us to transition to using namespaced classes. getInstance served as a service locator originally when we were not using PSR-4 autoloaders. Now that we are it's surplus to requirements and even actively causing issues with us using namespaced classes.

Testing Instructions

Ensure the entire CMS in all extensions continues to work exactly the same way. There shouldn't be any side effects of this PR - it's purely paving the way for future 5.0 development when the namespace maps will be moved into a compat plugin.

Link to documentations

Please select:

• Documentation link for docs.joomla.org:

• No documentation changes for docs.joomla.org needed

• Pull Request link for manual.joomla.org:

• No documentation changes for manual.joomla.org needed

[SharkyKZ]

Additionally it phases out the use of \Joomla\CMS\Table\Table::getInstance in favour of directly instantiating the class.

This is a B/C break for extensions. Joomla\CMS\Table\Table::getInstance() can fetch services from the container, which allows 3PDs to override table classes, including core ones. With your changes this functionality is lost. I get that there are already some places where classes are instantiated directly, but if anything, they should be replaced with consistent use of Joomla\CMS\Table\Table::getInstance() at least for the remainder of 4.x (and actually 5.x according to new policies...).

[wilsonge]

This is a B/C break for extensions. Joomla\CMS\Table\Table::getInstance() can fetch services from the container, which allows 3PDs to override table classes, including core ones

I think this is a fair point. I'd also argue that the way that acts currently returning a static cached instance - whereas the rest of getInstance does not is at best not consistent and at worst probably just outright broken (thinking of where within a table class I've currently replaced things with new static. It's only the models (JModelLegacy and JTable) where this is an issue from the original implementation - everything else was statically cached and therefore behaviour between dic and the original methods makes sense 7421a3d

But if you accept that as an argument (accept it's not so clear cut) then it's hard to find a way for people to override the classes.

[SharkyKZ]

I'd also argue that the way that acts currently returning a static cached instance

Not entirely true. That depends on how the service is defined. It might be shared or not.

[laoneo]

@wilsonge want to rebase this one to 5.0 or make it BC?

[wilsonge]

@HLeithner can you make a decision one way or another about this. Frankly I don't care that much whether we stick with the static getInstance or not (per @SharkyKZ 's comment) but we should at least be consistent.

If you're happy with this approach then I'll go back through and change all the static's to self's per the comments above. But don't wanna do that work if we decide to stick with the getInstance stuff for better b/c

[HLeithner]

plan is to get rid of getInstance but in a b/c way till 6.0, but if I see it correctly you always instance the same object again so I would not use new Category for example instead you new static() so that should have the same effect as if your use getInstance for this because getInstance would also resolve to the same implementation.

[wilsonge]

Current getInstance allows you to override the class through the container

joomla-cms/libraries/src/Table/Table.php

Lines 338 to 340 in 0ac412d

	if (Factory::getContainer()->has($tableClass)) {
	return Factory::getContainer()->get($tableClass);
	}

. I think before 6.0 we should either convert other things back to getInstance or move all core usage over to creating self/static. Having some things doing named classes and others doing getInstance is inconsistent

[HLeithner]

Yes I know I used this in the installer to fix cycle dependencies or where I was not able to inject the database object.
The question is, is this expected behavior? Our Container is not a Container it's a service locator right? which shouldn't store random objects.

In context of our use case in the store function of the banner table

            // Verify that the alias is unique
            /** @var BannerTable $table */
            $table = Table::getInstance('BannerTable', __NAMESPACE__ . '\\', ['dbo' => $db]);

it make zero sense to use getInstance as it could return a singleton because it would return self if someone set 'BannerTable' (with namespace) in the container and then call the store method on the same object.

At least for the getInstance calls within the Table object it should be new static

for the rest of the cms I'm unsure, it's a border case which maybe does more harm then good.

[wilsonge]

Our Container is not a Container it's a service locator right?

No the primary container object is supposed to be a genuine container. The individual component containers however are a service locator.

it make zero sense to use getInstance as it could return a singleton

Despite the name getInstance never returns a singleton (unless someone has explicitly made the container override a singleton - but that's them doing their own thing anyway)

for the rest of the cms I'm unsure, it's a border case which maybe does more harm then good.

Well bring it up in the next maintainers call or something. We need to be consistent. I think it's fine removing the containers which I doubt anyone is really using (and I showed you how to sort it in the installer properly) in the major version but I don't feel super strongly about that part. Just the fact we should have a documented consistent approach to how we use this stuff

[wilsonge]

@HLeithner what's the plan here?

[wilsonge]

Fixed the only outstanding feedback here. Please either merge or close and I'll create a PR to move back to Table::getInstance in the other places

[HLeithner]

We will see if it's a problem since this override way not always worked and in introduced in j4 I would say by error , I think it's not really used out there. We need migration documentation at manual please.