Draft: Implement new loadConfig() and localConfigCached() methods

[amulet1]

DRAFT

Proposed initial code changes based on #49:

• I did not remove anything yet, just added loadConfig() and loadConfigCached() both using internal static loadConfigInternal() method.
• Both functions return variable value (if single variable was requested) or associative array with valuable values. This simplifies usage of the methods, as this is exactly what the existing cases are after.
• If $vars = null, then all loaded variables are returned. This is mostly useful for caching (but we may not even need it - see below).
• Output is no longer captured, but it also means it will be emitted on loading. I do not see places where it is used.
• Note, I use a function (see $load_vars) that isolates loaded variables from all other variables, and therefore we only capture what is really loaded. Before compact was used to retrieve only the variables we are after, but the new code allows to retrieve (and possible cache) all the loaded variables.
• I did not test it, but it is probably very close to the final version.
• We still need to decide whether we want to use version with cache. In my opinion it is not needed, unless we start using long-term cache with auto-refresh (but this is somewhat beyond the current scope).

Typical usage:
Instead of $backends = $registry-> Horde::loadConfiguration('backends.php', 'backends', 'gollem');:
$backends = $registry->loadConfig('backends.php', 'backends');

Another example:

    public static function loadBackends()
    {
        global $registry;

        $config = $registry->loadConfigFile('backends.php', 'backends', 'ingo');
        if (empty($config->config['backends']) ||
            !is_array($config->config['backends'])) {
            throw new Ingo_Exception(_("No backends configured in backends.php"));
        }

        $out = [];
        foreach ($config->config['backends'] as $key => $val) {
            if (empty($val['disabled'])) {
                $out[$key] = $val;
            }
        }

        return $out;
    }

becomes

    public static function loadBackends()
    {
        global $registry;

        $backends = $registry->loadConfig('backends.php', 'backends', 'ingo');
        if (empty($backends) || !is_array($backends)) {
            throw new Ingo_Exception(_("No backends configured in backends.php"));
        }

        $out = [];
        foreach ($backends as $key => $val) {
            if (empty($val['disabled'])) {
                $out[$key] = $val;
            }
        }

        return $out;
    }

[amulet1]

@TDannhauer Please review.

[ralflang]

I like the idea very much! However:

@amulet1 Can you make it an injectable / autowirable class object rather than a static? Better for unit testing / mocking
@TDannhauer Any comments / thoughts? I would like to merge this as soon as makes sense.

[amulet1]

Note, loadConfig and loadConfigCache methods are not static, they only rely on static loadConfigInternal method.

Is this still a problem for unit testing?

[ralflang]

-- duplicate --

[ralflang]

Critical bug on line 1865: $this->get() called in static method will fatal error.

Fix by using $GLOBALS['registry'] instead:

        // Load global configuration file.
        if (defined('HORDE_CONFIG_BASE')) {
            $conf_dir = HORDE_CONFIG_BASE . DIRECTORY_SEPARATOR . $app . DIRECTORY_SEPARATOR;
        } else {
            if ($app == 'horde' && defined('HORDE_BASE')) {
                $conf_dir = HORDE_BASE . '/config/';
            } else {
                $conf_dir = $GLOBALS['registry']->get('fileroot', $app) . '/config/';
            }
        }

Alternative: Make loadConfigInternal() non-static and update callers from self:: to $this->.

[amulet1]

This is an unmerged/untested draft. And even if merged, it is not going to be used anywhere.

I will fix.

[amulet1]

Things to discuss/agree on:

1. We still need to decide regarding cache - either we (always) use it or never. I do not think we need both versions. I need some guidance on that.

2. Also, some existing code encloses original loadConfigFile into try ... catch block, and then error is either ignored or a custom error action is done. Let's add an optional boolean noError flag controlling whether to through error (default) or return null if no config files are found. This would add 4th parameter to the function which may not be convenient. But since we are not supporting PHP 7.4 anymore, named parameter could be used, if needed.

3. If we adopt this, the next things would be start replacing existing code with the new implementation and then removing the old functions for good.

4. Anything else?

[amulet1]

Waiting for the feedback...

[ralflang]

I don't get the big picture here.

Why new registry methods when it already does too much and needs a wholesale refactor?
Why do we no longer capture output and risk logging secrets and credentials?

I think I just need to understand what you want to achieve here.

In the bigger picture, registry should only be concerned with application paths and maybe the inter-app API. Anything else needs to be separated, especially config loading, authentication, permission checking, locale initialization, backend logins (imap!), notification system setup.

If we have an urgent need to change backend loading, let's consider.
Otherwise let's put it into a new, separate system in src/ which we can use for the PSR-15 & route based parts.

[amulet1]

Why new registry methods when it already does too much and needs a wholesale refactor?

This is because the original intention was to replace Horde_Registry::loadConfigFile() with a better version, and also get rid of all other variations that load/cache config files. Also, the existing loadConfigFile() stores cached data in the registry.

Why do we no longer capture output and risk logging secrets and credentials?

(1) I will add capturing/discarding output (with a warning). It was not clear why the output was captured (but never used) in the original implementation.

I think I just need to understand what you want to achieve here.

See #49.

In the bigger picture, registry should only be concerned with application paths and maybe the inter-app API. Anything else needs to be separated, especially config loading, authentication, permission checking, locale initialization, backend logins (imap!), notification system setup.

If we have an urgent need to change backend loading, let's consider. Otherwise let's put it into a new, separate system in src/ which we can use for the PSR-15 & route based parts.

There is no urgent need, just an idea to finally get rid of old code and consolidate config loading to use a simpler call. The modernization could be done later.

(2) I will move it to a separate class (with its own cache). The existing code could still called it via the registry method stub (I prefer not to use globals).

Would it be okay with (1) and (2) implemented?

[amulet1]

So, I guess we do not want to rely on this after all, given the #65 (was AI used there?).

While #65 looks modern (but not fully complete) it does not help to achieve any of the goals that were not achievable (or achieved) already.

While this sounds a little more conservative, I would prefer that we focus on fixing what is broken, on consolidating existing various similar implementations into optimized versions, migrating to already existing (but still unused modernized code). This all will help to produce stable releases (finally!).

Among our important goals I see that we want to improve stability, not break anything existing without a good reason and provide sufficient backward compatibility. This would help to avoid breakage due to potentially overlooked things, and increase overall stability. And if adding modern stuff helps with that, but all means let us do it).

[ralflang]

Hi,

I think we are not so far apart.

I don't see how the new API affects stability as it does not touch any existing system. The problem with trying to "fix" the old bootstrap process with lib/core.php (not a class), reliance on Horde_Autoloader, unexpected require_once in relative paths and recurring double definition of classes is we tried that since 2022 and it's leading nowhere.

So I'd rather build a new API without the Horde 3/Horde 4 constraints we no longer have and use it on some peripheral features, hammer out any bugs and issues, evolve it and when there is enough confidence, increase exposure.

I see your tremendous work in ActiveSync and Forms and it's really hard to both make these old interfaces work with new PHP versions, new features and use cases but also not break anything. It's impressive how rarely anything breaks in this process. I want to make sure we eventually have an easier life so I am gradually sifting through my non-public work from the maintaina era when collaborating with other colleagues on non-groupware use cases of the framework. It's not like what you see recently pops up out of nowhere.

I also tried to incorporate some of your ideas but not the caching right now. It's something we are going to want, I see it. We want to cache/compile the routes and also the registry config to reduce the complexity we carry with each request.

We have little room in the old registry framework - everything relies on everything, there's globals and undocumented expectations everywhere. I have been throwing out pieces from core.php for years and it always came with some breakage and guessing. It's so tiresome I'd rather leave it mostly untouched until we no longer need it at all. But if this new loader method helps bridge the gap between current core and a future core, let's do it.