? Pending

User tests: Successful: Unsuccessful:

avatar laoneo
laoneo
23 May 2018

Renames the \Joomla\CMS\HTML\Registry to \Joomla\CMS\HTML\HTMLRegistry.

Merge on review @wilsonge.

avatar laoneo laoneo - open - 23 May 2018
avatar laoneo laoneo - change - 23 May 2018
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 23 May 2018
Category Administration com_admin com_content Libraries
avatar mbabker
mbabker - comment - 23 May 2018

Again, why? Not much has changed since #17672.

If Registry is considered too ambiguous then Joomla\Registry\Registry should also be renamed. If the issue is there might be too many classes where the short name is Registry, that's a non-issue because namespaces are intended to separate these things.

Pick one class naming convention and stick with it. Either we prefix/suffix all class names with something from the previous namespace segment (i.e. HTML\HTMLRegistry or Controller\BaseController) or we don't (i.e. Service\Provider\Application or Http\Response) but pick one and be consistent throughout the code with it as a convention/standard. Anything short of that is merely nitpicking details for the sake of nitpicking details.

avatar laoneo
laoneo - comment - 23 May 2018

It has nothing to do with #17672.

This pr changes things actually to the current approaches we have to give classes a meaningful name. Otherwise we would have Controller\Base and not Controller\BaseController or MVC\Factoryand not MVC\MVCFactory.

avatar mbabker
mbabker - comment - 23 May 2018

The FQCN (Joomla\CMS\MVC\Controller\BaseController or Joomla\Http\Response) IS the meaningful class name. That's kind of my point. The short name (BaseController or Response) doesn't necessarily have to be meaningful entirely on its own (look at how the container service providers are named as an example).

If we really want the short name to be a meaningful class name on its own, then we need to rename a bunch more classes than this one Registry. That's why I'm saying pick a standard and stick with it so we can all adapt accordingly because right now there are mixed uses and we're just going to sit here and argue over whether class names are appropriate without a core standard.

(As I said in #17672 I still think ServiceRegistry is a more meaningful name than HTMLRegistry, it's a registry of services for the HTMLHelper::_() processor (I don't think HTMLHelper was the "best" class name for this either but too late to do anything on that one), it's not a registry of HTML "things", especially as the entire API has been misused and isn't entirely a set of services creating some kind of HTML output anymore)

avatar laoneo
laoneo - comment - 23 May 2018

A getRegistry function in a trait is for me too generic. I fear that it will lead sooner or later to clashes. Then I started to rename things and one change lead to the next one and it ended up with a rename to HTMLRegistry.

avatar mbabker
mbabker - comment - 23 May 2018

getX() doesn't have to return a class named X.

To be honest, the intent really wasn't to have that registry accessible as a global service, but as an internal detail to HTMLHelper. So looking at some of this code for the first time in a while, I feel like some of it actually is complicating matters a bit more than needs to be. Unless you've got a master plan for making HTMLHelper::_() non-static (which you basically need Laravel style Facades for, that idea has already been proposed and shot down because it's too technically complex for the Joomla user base), I'd honestly back out some of those changes and just have the service layer call HTMLHelper::registerService('foo', $thing); versus trying to put that in the container.

avatar laoneo
laoneo - comment - 23 May 2018

I was playing a bit around to see how far I could go with injecting the Registry into a view, I made #20551. Actually it would be nice to get rid of HTMLHelper and to have some none static way to load these.

avatar mbabker
mbabker - comment - 23 May 2018

Actually it would be nice to get rid of HTMLHelper and to have some none static way to load these.

I'm going to be honest here. Our layout rendering engines need to go and need to be replaced with something like Twig or Blade or Plates.

Most of what's used for JHtml helpers really should be a Blade directive or a Twig function/macro or a Plates function/extension. So in the rendering layer you're calling to a registered service of the templating engine, you're not calling to a global service that has been misused and abused.

Granted, with pure PHP we can't prevent layout files from misusing the PHP API, but we really need to make some hard decisions at some point and find a way to move to a proper decoupled rendering solution. The fact that in a component layout file I have direct access to the entire JViewLegacy API to do anything I want is bad, the layout file should ONLY have access to injected data.

avatar laoneo
laoneo - comment - 23 May 2018

Agree, that a proper template engine should be used instead of our PHP layouts. But we have to deal with what we have now and need to find a way to improve it as it is very unlikely to get rid of it.

avatar laoneo
laoneo - comment - 23 May 2018

#13684 was a way into the injected direction.

avatar mbabker
mbabker - comment - 23 May 2018

But we have to deal with what we have now and need to find a way to improve it as it is very unlikely to get rid of it.

Because every attempt to propose needed changes in the MVC layer gets shot down because of B/C concerns, and every attempt to introduce a new MVC layer in parallel where B/C isn't as major of a concern gets shot down. Sooner or later we're all going to wise up and realize you can't keep monkey patching a broken architecture. Or, we all need to just accept that the architecture is locked in the state it is because Joomla doesn't have the resources to adequately build it out the way it needs to be done.

avatar laoneo laoneo - change - 28 May 2018
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2018-05-28 08:29:36
Closed_By laoneo
Labels Added: ?
avatar laoneo laoneo - close - 28 May 2018

Add a Comment

Login with GitHub to post a comment