Skip to content

[4.0] Offer components to register their HTML services in the service provider#19840

Merged
wilsonge merged 19 commits intojoomla:4.0-devfrom
Digital-Peak:j4/service/jhtml
Mar 19, 2018
Merged

[4.0] Offer components to register their HTML services in the service provider#19840
wilsonge merged 19 commits intojoomla:4.0-devfrom
Digital-Peak:j4/service/jhtml

Conversation

@laoneo
Copy link
Member

@laoneo laoneo commented Mar 5, 2018

Summary of Changes

With the introduction of a service registry for HTML services in #17328, components do need a way to register their own HTML services during the dispatch phase.

This pr introduces a HTML Registry service provider. Through this service, an HTML registry can be injected into the Component dispatchers where they can load their own services in the function loadHTMLServices.

Testing Instructions

Open an article on the front end.

Expected result

The print and email icons should be visible.

Actual result

The print and email icons should be visible.

@laoneo laoneo force-pushed the j4/service/jhtml branch from fb2ebcb to 50f1100 Compare March 5, 2018 13:54
@mbabker
Copy link
Contributor

mbabker commented Mar 5, 2018

Maybe I'm missing something obvious, but why does this need to be part of the component dispatcher and not something that exists closer to the service configuration (I'd expect this step to be something I do in https://github.com/joomla/joomla-cms/blob/4.0-dev/administrator/components/com_content/services/provider.php personally)?

@laoneo
Copy link
Member Author

laoneo commented Mar 5, 2018

During development I was thinking about this approach. The problem comes when you boot a component from another component which has the same HTML services. It will then overwrite to the services from the dispatched one. From what I saw in the code is that the services are only used during dispatch and not after the boot process already.

$container->set(DispatcherFactoryInterface::class, new DispatcherFactory('\\Joomla\\Component\\Content'));
$container->set(
DispatcherFactoryInterface::class,
new DispatcherFactory('\\Joomla\\Component\\Content', $container->get(Registry::class))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think doing Registry here is the right thing - we're using this for a tonne of configuration options. In a global container this is far too generic.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be the FQCN (Joomla\CMS\HTML\Registry), not the literal string "Registry"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops - I misread the import 😊

@wilsonge
Copy link
Contributor

wilsonge commented Mar 5, 2018

The problem comes when you boot a component from another component which has the same HTML services. It will then overwrite to the services from the dispatched one

Why would this happen - they would be in different containers?

@mbabker
Copy link
Contributor

mbabker commented Mar 5, 2018

The HTML registry is global.

@laoneo
Copy link
Member Author

laoneo commented Mar 6, 2018

Perhaps we should rename the Registry to HTMLRegsitry on a second step.

@wilsonge
Copy link
Contributor

wilsonge commented Mar 9, 2018

The dispatcher is the wrong place to be registering this. If I now get a mvcfactory from bootComponent then none of the Html services are available. This needs to be registered in the components services/provider.php file

I think probably best is to add a new interface or something that it can implement that contains a similar method to loadHTMLServices

@laoneo
Copy link
Member Author

laoneo commented Mar 9, 2018

The only concern I have is that it can happen that a component gets booted and registers the same HTML services as the dispatched component. Then they get overwritten. Thats the only reason why I put them into the dispatcher. As long as there is one global HTML registry we have that problem.

@mbabker
Copy link
Contributor

mbabker commented Mar 9, 2018

It's only a problem because the component helpers aren't autoloaded which lets you have X number of JHtmlIcon classes (IIRC we have at least 3) since the code runs under the assumption that presumably only one component will ever be executed in a request cycle and the odds of another component's stuff even being loaded are slim to none. I get that we have a lot of code to make the CMS lazy load as much as possible, but sooner or later that extra lazy loading logic is going to bite us in the backside. This is one of those cases where it bites in the backside.

@laoneo
Copy link
Member Author

laoneo commented Mar 9, 2018

I don't want to get bitten. As long as we do not have a solution for that problem, we can't put it into the service provider. Or you guys want to take the risk?

@mbabker
Copy link
Contributor

mbabker commented Mar 9, 2018

At some point there has to be a B/C break in whatever core components have a JHtmlIcon class to rename it (and presumably the third party ecosystem too if they've also got one of those classes in their extensions) to avoid later on running into duplicate class name issues or the duplicate service name in the HTML registry. When do we want to make it?

@wilsonge
Copy link
Contributor

wilsonge commented Mar 9, 2018

I still think we should make them "namespaced" (like we had the triple ContentHtmlIcon stuff) now rather than later

@laoneo
Copy link
Member Author

laoneo commented Mar 9, 2018

How should we name then the service? JHtml::_('contenticon.edit'); ?

If we want to do a BC break then now when there is a need for.

…ce/jhtml

# Conflicts:
#	administrator/components/com_content/services/provider.php
#	libraries/src/Dispatcher/Dispatcher.php
#	libraries/src/Dispatcher/DispatcherFactory.php
@wilsonge
Copy link
Contributor

wilsonge commented Mar 9, 2018

Well until Michael did the registry you could already do that iirc. I think when we merged the service registry we chose to not have that work with the registry.

@mbabker
Copy link
Contributor

mbabker commented Mar 9, 2018

The registry removes the class name structure (JHtml<foo> for a two part key like foo.thing, <foo><bar> for a three part key like foo.bar.thing), it only requires unique identifiers within the registry itself and the class names can be anything (not much different than any other service provider/container).

@laoneo laoneo force-pushed the j4/service/jhtml branch from 201ca6e to dd40bd3 Compare March 9, 2018 19:04
@laoneo laoneo force-pushed the j4/service/jhtml branch from dd40bd3 to ad22ea3 Compare March 9, 2018 19:05
@laoneo
Copy link
Member Author

laoneo commented Mar 9, 2018

So I reverted the injected registry and instead of it gets filled in the service provider. But somehow it looks for me awkward to fill the registry in a service provider.

@mbabker
Copy link
Contributor

mbabker commented Mar 9, 2018

But somehow it looks for me awkward to fill the registry in a service provider.

It's not that weird. Something has to fill the registry so that the registry is aware of what it provides. A registry is more often than not just a fancy service locator for a specific purpose, so when you look at it that way it makes sense that in your service building code you add resources that the service registry can resolve later.

laoneo added 2 commits March 10, 2018 07:11
…ce/jhtml

# Conflicts:
#	administrator/components/com_content/services/provider.php
@laoneo laoneo changed the title [4.0] Offer components to register their HTML services in the dispatcher [4.0] Offer components to register their HTML services in the service provider Mar 11, 2018
@wilsonge wilsonge merged commit 4487099 into joomla:4.0-dev Mar 19, 2018
@wilsonge wilsonge deleted the j4/service/jhtml branch March 19, 2018 14:15
@zero-24 zero-24 added this to the Joomla 4.0 milestone Mar 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants