? ? Pending

User tests: Successful: Unsuccessful:

avatar laoneo
laoneo
5 Mar 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.

avatar laoneo laoneo - open - 5 Mar 2018
avatar laoneo laoneo - change - 5 Mar 2018
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 5 Mar 2018
Category Administration com_content Front End Libraries
avatar laoneo laoneo - change - 5 Mar 2018
Labels Added: ?
0675754 5 Mar 2018 avatar laoneo CS
avatar mbabker
mbabker - comment - 5 Mar 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)?

avatar laoneo
laoneo - comment - 5 Mar 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.

avatar wilsonge
wilsonge - comment - 5 Mar 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?

avatar mbabker
mbabker - comment - 5 Mar 2018

The HTML registry is global.

avatar laoneo
laoneo - comment - 6 Mar 2018

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

avatar joomla-cms-bot joomla-cms-bot - change - 8 Mar 2018
Category Administration com_content Front End Libraries Administration com_content Front End Libraries Unit Tests
avatar laoneo laoneo - change - 8 Mar 2018
Labels Added: ?
avatar wilsonge
wilsonge - comment - 9 Mar 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

avatar laoneo
laoneo - comment - 9 Mar 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.

avatar mbabker
mbabker - comment - 9 Mar 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.

avatar laoneo
laoneo - comment - 9 Mar 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?

avatar mbabker
mbabker - comment - 9 Mar 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?

avatar wilsonge
wilsonge - comment - 9 Mar 2018

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

avatar laoneo
laoneo - comment - 9 Mar 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.

avatar wilsonge
wilsonge - comment - 9 Mar 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.

avatar mbabker
mbabker - comment - 9 Mar 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).

avatar laoneo
laoneo - comment - 9 Mar 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.

avatar mbabker
mbabker - comment - 9 Mar 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.

avatar laoneo laoneo - change - 11 Mar 2018
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
avatar laoneo laoneo - edited - 11 Mar 2018
avatar wilsonge wilsonge - close - 19 Mar 2018
avatar wilsonge wilsonge - merge - 19 Mar 2018
avatar wilsonge wilsonge - change - 19 Mar 2018
Status Pending Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2018-03-19 14:15:13
Closed_By wilsonge

Add a Comment

Login with GitHub to post a comment