loadConfiguration and loadConfigFile

[amulet1]

We have (A) Horde_Deprecated::loadConfiguration() which is still used in several places. Internally it is rewritten to call (B) Horde_Registry::loadConfigFile(), which caches and returns instance of (C) Horde_Registry_Loadconfig class. Also Horde_Registry_Loadconfig is called directly in a few places.

I see several issues:

1. Why do we still use the supposedly deprecated loadConfiguration()?
2. It is questionable that cache in Horde_Registry::loadConfigFile() really helps with anything. If we indeed load same file multiple times in the same app during a single request, than maybe something is wrong with the app. Additionally, the result depends on the $vars parameter, however the cache key disregards this, and if a call is ever made for the same config file but with different $vars parameter, it will return the previously cached value (wrong!).
3. The main reason Horde_Registry_Loadconfig instance is returned is to be able to access its output and config properties. Its constructor uses $registry global, which could be avoided if it was implemented as a method in Horde_Registry class.
4. I looked at all places where the 3 methods are used across all Horde applications. As far as I can see, output property (which holds captured output produced by loaded file) is never used. I am not sure if there are some 3rd party applications that somehow rely on it, but clearly there are simpler ways to capture output. Places that use (B) or (C) use config property value - which is what the deprecated Horde_Deprecated::loadConfiguration() method already did.

In nearly all cases (when a single variable was used) the "deprecated" code looked like
(D) $result = Horde::loadConfiguration($config_file, $var_name, $app);

The equivalent newer code is
(E) $result = $registry->loadConfigFile($config_file, $var_name, $app)->config[$var_name];

Suggestions:
Let's create a new (non-static!) method Horde_Registry:loadConfig which would generally replicate (A) behavior, but without any extra classes or methods involved, and without output being captured.

We have to decide if caching is needed (probably not, or at least it should be optional). If we need it, then at least it should be done smarter - e.g. cache values in Redis, and auto-refresh if file(s) last-modified timestamp changes.

We then switch to using this method instead of (A), (B) and (C) and remove the unused old code.

With this suggestion a typical call equivalent to (D) or (E) would be like this:
(F) $result = $registry->loadConfig($config_file, $var_name, $app);

Overall (F) is cleaner and simpler to use.

Any thoughts?

[amulet1]

@TDannhauer Please comment. if you agree, I will produce PR(s).

[TDannhauer]

@amulet1 : Sounds for me like another case of starting to change things without applying it properly to all affected corners so that modernization project ended premature between start and full cleanup.

We need to keep in mind that Horde was initially a framework, so it might offer more functionality that it is used in the core apps.
But as I said recently: I regard horde as close to abandon ware. therfore: lets now keep every tiny functionality as it was. Third party app maintainers anyway need to adapt to H6, and we take care for the core apps ourself.

Therfore my general approach/mindset is as follows:

1. ensure changes are conssiten and we do not add another case of premature ended modernization
2. dare to cut of support of very old bowsers and unused functionality. Better a leaner and (from framework perspective) less functional sbut up toe date and maintained software.

Is it fitting into your plans/ideas/approaches?

Can you provide PRs?

[amulet1]

Is it fitting into your plans/ideas/approaches?

Yes, more or less. If I see something that was half-finalized, and it can also be improved (and be leaner/simpler to use) I am all for making it better.

And, I would prefer to start removing obsolete code even if it might accidentally break some old 3rd party apps (as you said, they would have to adapt to H6, anyway). It might be important to document any breaking changes though, but I am not so good at documenting things.

Can you provide PRs?

OK, I will.