I noticed that HTMLHelper::_() registers methods if they're not registered yet. Maybe HTMLHelper::isRegistered() should do the same?

An isser should not act as a setter.

And HTMLHelper::_() should?

With the way the API was originally designed (runtime lookups for static functions in classes following a specific naming structure), the way the underscore method behaves is honestly the most efficient way to do it. Deprecating the path lookup feature and the per-method key registration in favor of service classes gets things to the point that in 5.0 that method should not be registering anything new on its own, everything should be registered beforehand using the registry and the registry should be used to handle overriding things in the same way you would override methods from PHP classes (by either extending a parent class or using the decorator pattern, the decorator pattern is probably better because in theory it allows multiple consumers to extend a single service without causing too many problems).

I just think this looks really dodgy:

if (JHtml::isRegistered('foo.bar'))
{
	// Is not executed.
	JHtml::_('foo.bar');
}

// Is executed and added to registry.
JHtml::_('foo.bar');

if (JHtml::isRegistered('foo.bar'))
{
	// Is now executed too.
	JHtml::_('foo.bar');
}

That's the side effect of supporting runtime resolution of the methods. Without that, you're in an even weirder spot because you effectively have to manually register every method in every class explicitly on every request, and that's a lot of performance overhead.

<?php

use Joomla\CMS\HTML\HTMLHelper;

foreach ((new ReflectionClass(JHtmlJquery::class))->getMethods() as $method) {
    // HTMLHelper effectively only supports static methods in this structure
    if (!$method->isStatic()) {
        continue;
    }

    // Callables must be public methods
    if (!$method->isPublic()) {
        continue;
    }

    HTMLHelper::register(sprintf('jquery.%s', strtolower($method->getName())), [JHtmlJquery::class, $method->getName()]);
}

BTW, totally unrelated, but see if this replacement for HTMLHelper::call() works OK without side effects, I think this should work OK for PHP 7.2+ without needing the temp array (and probably should be a bit more performant):

protected static function call(callable $function, $args)
{
	// Alternatives if that's too problematic:
	// return \call_user_func($function, ...$args);
	// return \call_user_func_array($function, $args);
	return $function(...$args);
}

HTMLHelper does register non-static methods at the moment. Having that in mind, a simpler get_class_methods() would be in line with how it works now.

Thanks for your answers but they seem to be targeted at 4.0 and beyond while I'm looking for a solution for 3.x. Should we just revert that part in #10882 to fix a specific case and just ignore that services aren't being registered properly or can we work on some workaround?

No, there isn't any good way to fix that specific case.

The problem is the known callbacks aren't registered until they are called (i.e. do JHtml::isRegistered('jquery.framework') or JHtml::isRegistered('date') in an onAfterInitialise hook, they'll both be false there). That goes back to the whole runtime resolution thing. To get that to return true, you would have to explicitly call JHtml::register('jquery.framework, [JHtmlJquery::class, 'framework']) somewhere in the Joomla bootup sequence. Same goes for every helper inside the JHtml class and all the helper classes in the libraries/cms/html path. That is a major performance hit to go through that routine. And that's only dealing with the library level helpers, you also have the component level helpers which are conditionally defined based on a request (such as those in com_users).

JHtml::addIncludePath() does not scan a directory and automatically register callbacks, that would also be an expensive performance penalty to do so.

Long and short, you cannot 100% rely on JHtml::isRegistered() until either the first time a helper is executed or there is an explicit register call elsewhere before first use (generally to override something).

The change in https://github.com/joomla/joomla-cms/pull/10882/files#diff-5f1cbd8b480fca338736dce8ebfe2248L12-R12 is correct given the 3.x API design. One should not be manually registering their HTML helper class to the autoloader AND manually registering each available callback via JHtml::register(). They should be using JHtml::addIncludePath() and dealing with the fact that a service isn't "registered" until something makes an explicit JHtml::register() call (either by first use or someone making a call from outside the JHtml class). Trying to do it any other way is going to cause a bigger performance hit to Joomla than the benefit of JHtml::isRegistered() actually being reliable would introduce. The registry API introduced into 4.0 is the most efficient way to fix this and if 5.0 rolls around then it will be the only way to deal with it.

Trying to do it any other way is going to cause a bigger performance hit to Joomla

I don't think so. We'd really need just the same logic as in the underscore method. And once service key is successfully registered, we'd pull it from the registry so no huge penalty here.

That would be a huge B/C break. And would become a burden for contributors as core would have to boot up the internal registry of known JHtml helper functions on every request and seed it with all of the core HTML helpers (the public methods of JHtml itself and all of the helper classes in libraries/cms/html). You either end up with dynamic code that introduces a very noticeable performance degradation or you end up with a massive array that is a PITA to maintain.

Here's the thing. Just like other parts of the API, addIncludePath() adds a lookup to the internal paths array where it attempts to find a file matching a convention (typically a file named something containing a class named Something) and operates in a LIFO order (so the last path you plug in is the first path searched). addIncludePath(), in any part of the Joomla API (Cache, HTML, Layout, Form, Log, and MVC) was never intended to autoload and auto-register services, because that's just not how Joomla functions. Those paths are used at runtime to attempt to resolve an unknown service.

This is why the right way to fix this problem is to deprecate the entire override mechanism of JHtml, as I have done with 4.0, and replace it with a service oriented architecture that is more in-tune with how PHP APIs are designed by modern standards.

Sorry, but I didn't suggest any of that. My idea is just to have isRegistered() return true if the method can be managed by HTMLHelper (same way _ method works), and perhaps optionally register it. If it's not suitable for this method (I do think it would be weird to have it return true after unregister() call), maybe a new method can be introduced.

And for 4.0, either I have no idea how to use, or there's really no way to check if method is registered with the new service registry, except using method_exists() or is_callable() type of checks in the layouts.

Calling isRegistered() should not attempt to register anything. It violates SRP and is architecturally incorrect behavior.

With the service registry, register() and isRegistered() start to go away. From the outside, there won’t be an API anymore to check if a specific method exists somewhere. It is a tradeoff to replacing the existing system, but it must be done.

You have already pointed out why the current method based system is fundamentally flawed (runtime resolution and an inconsistent state in the API). There really is no saving the current method based system without major performance penalties, purposeful API design that violates most modern standards, or creating a system that is unmaintainable for the core contributors.

If you really want the current method approach to work, there's a way to do it. This is the basic prototype for the relevant methods in Joomla\CMS\HTML\HTMLHelper and Joomla\CMS\HTML\Registry without any compatibility layer, if you want to see it happen it shouldn't be much effort to work back in all the other stuff that should be deprecated. The biggest thing here though is that the behavior of addIncludePath() does not change AT ALL, it just needs to go away for being the relic of days past when Joomla had no idea what an autoloader or service oriented architecture was (because remember at one point before autoloading the API design was to loop over the paths in LIFO order, find a file that matches convention, and use it; this effectively allowed a full class override of a service (i.e. JHtmlJquery could be overridden with a separate class), but this had the cost to people doing it that they must keep up with all methods in the overridden class on their own).

The other key thing is to make things like isRegistered() actually work 100% of the time every time, the core helper methods MUST get registered into the system somehow. The most efficient way to do this is to populate the registry at first access, but this is where the performance penalty comes and where a lot of testing and iteration will need to come to make sure that the most optimal method is used. With this API design, instead of registering a path to find the right file a little later on, you're registering either an explicit method callable or a class (either a class name or instance) that is immediately known.

This also probably needs a memory benchmark because I imagine a service class holding over 100 callables in memory might have some memory requirements.

namespace Joomla\CMS\HTML;

use Joomla\CMS\Factory;

final class HTMLHelper
{
    /**
     * The service registry containing registered helper functions
     */
    private static Registry $serviceRegistry;

    private function __construct()
    {
        // This class cannot be instantiated or extended
    }

    private static function extract(string $key): string
    {
        $key = preg_replace('#[^A-Z0-9_\.]#i', '', $key);

        $parts = explode('.', $key);

        if (\count($parts) > 2) {
            throw new \InvalidArgumentException(sprintf('Method keys for %s may only contain up to one dot, "%s" is an invalid method key.', self::class, $key));
        }

        return $key;
    }

    /**
     * @param mixed ...$methodArgs The arguments to pass forward to the method being called
     *
     * @return mixed Result of HTMLHelper::call($function, $args)
     *
     * @throws \InvalidArgumentException
     */
    public static function _(string $key, ...$methodArgs)
    {
        $key = static::extract($key);

        if (static::getServiceRegistry()->has($key)) {
            return static::call(static::getServiceRegistry()->get($key), $methodArgs);
        }

        // Deprecated path lookup logic would go here
        throw new \InvalidArgumentException(sprintf('The "%s" key is not registered in %s.', $key, self::class));
    }

    /**
     * @throws \RuntimeException if the key is already registered and the $replace flag is not set to true
     */
    public static function register(string $key, callable $handler, bool $replace = false): void
    {
        static::getServiceRegistry()->register($key, $handler, $replace);
    }

    /**
     * @param string|object $classNameOrInstance A string representing a class name for a service, or a class instance
     *
     * @throws \InvalidArgumentException if the $classNameOrInstance parameter is not a string or an object, or if a string the class does not exist
     */
    public static function registerService(string $key, $classNameOrInstance, bool $replace = false): void
    {
        static::getServiceRegistry()->registerService($key, $classNameOrInstance, $replace);
    }

    public static function unregister(string $key): void
    {
        static::getServiceRegistry()->unregister($key);
    }

    public static function isRegistered(string $key): bool
    {
        $key = static::extract($key);

        return static::getServiceRegistry()->has($key);
    }

    public static function getServiceRegistry(): Registry
    {
        if (!static::$serviceRegistry) {
            static::$serviceRegistry = Factory::getContainer()->get(Registry::class);
        }

        return static::$serviceRegistry;
    }

    private static function call(callable $function, $args)
    {
        return $function(...$args);
    }
}

final class Registry
{
    /**
     * Mapping array of the core CMS HTML helper classes.
     */
    private const CORE_HELPER_CLASSES = [
        'access'          => \JHtmlAccess::class,
        'actionsdropdown' => \JHtmlActionsDropdown::class,
        'adminlanguage'   => \JHtmlAdminLanguage::class,
        'behavior'        => \JHtmlBehavior::class,
        'bootstrap'       => \JHtmlBootstrap::class,
        'category'        => \JHtmlCategory::class,
        'content'         => \JHtmlContent::class,
        'contentlanguage' => \JHtmlContentlanguage::class,
        'date'            => \JHtmlDate::class,
        'debug'           => \JHtmlDebug::class,
        'draggablelist'   => \JHtmlDraggablelist::class,
        'dropdown'        => \JHtmlDropdown::class,
        'email'           => \JHtmlEmail::class,
        'form'            => \JHtmlForm::class,
        'formbehavior'    => \JHtmlFormbehavior::class,
        'grid'            => \JHtmlGrid::class,
        'icons'           => \JHtmlIcons::class,
        'jgrid'           => \JHtmlJGrid::class,
        'jquery'          => \JHtmlJquery::class,
        'links'           => \JHtmlLinks::class,
        'list'            => \JHtmlList::class,
        'menu'            => \JHtmlMenu::class,
        'number'          => \JHtmlNumber::class,
        'searchtools'     => \JHtmlSearchtools::class,
        'select'          => \JHtmlSelect::class,
        'sidebar'         => \JHtmlSidebar::class,
        'sortablelist'    => \JHtmlSortablelist::class,
        'string'          => \JHtmlString::class,
        'tag'             => \JHtmlTag::class,
        'tel'             => \JHtmlTel::class,
        'uitab'           => \JHtmlUiTab::class,
        'user'            => \JHtmlUser::class,
        'workflowstage'   => \JHtmlWorkflowstage::class,
    ];

    /**
     * Array holding the registered services.
     *
     * @var array<string, callable>
     */
    private array $serviceMap = [];

    private bool $initialised = false;

    public function initialise(): void
    {
        if ($this->initialised) {
            return;
        }

        // Register all callables for core helpers, this is the part that needs to be performance optimized somehow
        foreach (self::CORE_HELPER_CLASSES as $key => $className) {
            $this->registerService($key, $className, true);
        }

        $this->initialised = true;
    }

    /**
     * @throws \InvalidArgumentException if the key is not registered
     */
    public function get(string $key): callable
    {
        if (!$this->initialised) {
            $this->initialise();
        }

        if (!$this->has($key)) {
            throw new \InvalidArgumentException(sprintf('The "%s" key is not registered.', $key));
        }

        return $this->serviceMap[$key];
    }

    public function has(string $key): bool
    {
        if (!$this->initialised) {
            $this->initialise();
        }

        return isset($this->serviceMap[$key]);
    }

    /**
     * @throws \RuntimeException if the key is already registered and the $replace flag is not set to true
     */
    public function register(string $key, callable $handler, bool $replace = false): void
    {
        if (!$this->initialised) {
            $this->initialise();
        }

        // If the key exists already and we aren't instructed to replace existing services, disallow registration
        if (isset($this->serviceMap[$key]) && !$replace) {
            throw new \RuntimeException(sprintf('The "%s" key is already registered.', $key));
        }

        $this->serviceMap[$key] = $handler;
    }

    /**
     * @param string|object $classNameOrInstance A string representing a class name for a service, or a class instance
     *
     * @throws \InvalidArgumentException if the $classNameOrInstance parameter is not a string or an object, or if a string the class does not exist
     */
    public function registerService(string $key, $classNameOrInstance, bool $replace = false): void
    {
        if (!\is_string($classNameOrInstance) && !\is_object($classNameOrInstance)) {
            throw new \InvalidArgumentException(sprintf('The $classNameOrInstance parameter for %s() must be a string or an object, a "%s" was given.', __METHOD__, \gettype($classNameOrInstance)));
        }

        if (\is_string($classNameOrInstance) && !class_exists($classNameOrInstance)) {
            throw new \InvalidArgumentException(sprintf('The "%s" class does not exist, cannot register service.', $classNameOrInstance));
        }

        foreach ((new \ReflectionClass($classNameOrInstance))->getMethods() as $method) {
            // Callables must be public methods
            if (!$method->isPublic()) {
                continue;
            }

            $this->register(sprintf('%s.%s', $key, strtolower($method->getName())), [$classNameOrInstance, $method->getName()], $replace);
        }
    }

    public function unregister(string $key): void
    {
        if (!$this->initialised) {
            $this->initialise();
        }

        unset($this->serviceMap[$key]);
    }
}

Out of memory. But issue seems to be coming from elsewhere, not those 100-200 callables.

Memory issue seems because of recursion. Registry::initialise() calls Registry::register() method which in turn runs Registry::initialise() again because Registry::$initialised hasn't been set yet. Is there a reason we can't register core library methods in the constructor? And is this even needed if we go this path? Populating registry on the fly would essentially allow it to act as some sort of cache.

In your honest opinion, do you think it's better off the way it is now and those 2 uses of HTMLHelper::isRegistered() we have in core (specifically in com_users) should just be refactored to use something else?

Is there a reason we can't register core library methods in the constructor? And is this even needed if we go this path?

Constructor forces the initialization to happen as soon as the object is instantiated. The way I prototyped it defers that initialization to first use. The Registry is going to be instantiated on every request thanks to the service layer, but in requests that are serving non-HTML responses (API requests, MVC requests that are doing a write action and redirect, com_finder indexer and its AJAX calls, etc.) then this is a bunch of extra overhead. So the lazy-load part of it probably needs some tweaking to get it to work right (I just prototyped it out over 20 minutes without actually trying to run it anywhere).

In your honest opinion, do you think it's better off the way it is now and those 2 uses of HTMLHelper::isRegistered() we have in core (specifically in com_users) should just be refactored to use something else?

There are merits in both the method based structure and the "pure" service based structure. I don't think either one is necessarily wrong, or even better than the other, but I do think the current (3.x) API design causes too many inconsistencies (just as you found out the semi-hard way with making those isRegistered() checks) to be relied on as is and needs some kind of refactoring.

Looking more into this, it kind of makes sense to not have service keys registered until first use. This way we know whether the key should be overridden or not (only unused key should be overridden, otherwise we run into various issues). So to me it would make sense to have separate methods for checking if the method has actually been used and if it's manageable by HTMLHelper.