User tests: Successful: Unsuccessful:
It would be nice when we can test somehow layouts. A step into that direction would be to get rid of static calls. This pr injects the (HTML) Registry into the view to get the services from. The services are then available through $this->getRegistry()->_()
in the layout.
For example
JHtml::_('content.prepare', $this->item->introtext);
becomes
$this->getRegistry()->_('content.prepare', $this->item->introtext);
.
When #13684 should get merged on some point in the future, we can then replace $this->getRegistry()->_()
with $registry->_();
.
Status | New | ⇒ | Pending |
Category | ⇒ | Administration com_content Front End Libraries |
Labels |
Added:
?
?
|
I didn't talk about unit testing. What would you suggest then instead of this pr? Just giving a
Based purely on this comment:
It would be nice when we can test somehow layouts. A step into that direction would be to get rid of static calls.
That basically says you're trying to fix a problem by creating a new problem, to me anyway.
I'd rather not try to fix the HTMLHelper::_()
static method call problem by injecting some service into a scope that is already too crowded. The root problem is the way our rendering layers are built allows way too much to happen in layouts that should not be layout responsibilities or actionable within layouts to begin with.
HTMLHelper
and its capabilities, as a global service or as a static API, is not something that should be a property of a JLayout
or JViewLegacy
object. Jump over to the HTMLRegistry PR for further context, but really what HTMLHelper
does should be an extension system of a templating engine, but we don't have a templating engine in core or a proper separation of concerns as it relates to view classes (which should be collecting data and forwarding that to the rendering system) and layouts in the MVC layer. Monkey patching things so that you remove static method calls doesn't really fix any of the real API design flaws, and I'd rather us stick with one suboptimal API that's already been present for a long time than move to another suboptimal API that doesn't really fix anything other than possibly making it easier to mock something with a rendering function.
Ok so lets go a step back. Assume we still keep the PHP layouts. How would you do ideally then write the following code when you can start from scratch with the Joomla CMS:
<div>
<?php echo HTMLHelper::_('my.service', ['arg' => 'foo']); ?>
</div>
Honestly, with plain PHP without any form of templating engine in place (essentially what Joomla has now, but take the MVC layer out of the equation), I'd make a static call direct to a helper method or a global PHP function (if you're doing truly plain PHP without any form of framework like a CMS offers). Because being able to use $this
in a layout file to call any form of service integration is to me a major code smell, even something as simple as $this->escape()
offered by our layout or view APIs creates trouble because it means layouts are exposed to whatever is in the class scope.
To me, a layout file should always be included by a function similar to this:
function scope_isolated_include(string $path, array ...$args)
{
// $args should be an associative array, extract to make its data available as variables
extract($args);
// And unset args, not needed in file
unset($args);
include $path;
}
The layout file is scope isolated and doesn't give you the ability AT ALL to do anything unless you can get to it with either native PHP or through some kind of static exposing your service layer (i.e. JFactory
, JHtml
, etc.).
HTMLHelper
you can't really do as template engine functions/extensions/integrations/whatever-you-wanna-call-it. So really, in templating engines you'd just have a function that proxies to the existing class. So in Plates it'd be $engine->registerFunction('html', [HTMLHelper::class, '_']);
, for Twig that'd be something like new TwigFunction('html', [HTMLHelper::class, '_'], ['is_safe' => ['html']])
, etc. etc. Because really HTMLHelper
is a runtime service resolver on its own accord (nothing stops you from registering a custom function to replace the date.relative
key, calling it, then immediately unregistering that afterwards).
So really, what's exposed to the rendering layer is a Facade like proxy, but HTMLHelper
isn't directly exposed to the renderer and you don't have to scope creep by pushing a service into the view class.
In the end I don't have an issue with making HTMLHelper
some kind of proper service registry and phasing out its statics. But I don't think that you make that particular API an OOP architecture first and try to adapt the existing architecture/API to that change. I think we need some higher level architecture changes first where it then makes sense to migrate HTMLHelper
away from a static facade type layer to proper services/OOP.
I think we need some higher level architecture changes first where it then makes sense to migrate HTMLHelper away from a static facade type layer to proper services/OOP.
Sounds like a good plan. So the biggest issue is that we have a scope in views. The approach of #13684 was to use layouts for that and execute them in an isolated scope. For me this is still a acceptable solution when we do some changes in the pr, to not hardcode the layout stuff in the BaseView, instead in a new LayoutView
which extends from HTMLView
. What do you think?
Status | Pending | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2018-06-20 11:04:20 |
Closed_By | ⇒ | laoneo |
Layouts aren't intended to be unit tested, so? on this. To test views/layouts you should be doing functional testing with real services, not mocks. If you're trying to unit test something with a view or layout class integration, you mock the view or layout class, but you really shouldn't be testing the rendered markup in unit tests except for very exceptional circumstances (i.e. a unit test for
HTMLHelper::link()
. What we did with testing markup in form fields as an example is not the way it should be done by any measure.Not to mention that this is MASSIVELY complicating the use of the HTML library, and we really need to stop introducing vague as all hell and terribly named methods like
_
.