Proposed improvements in Horde_Array and Horde\Util\ArrayUtils

[amulet1]

There are several functions which should be improved:

• Some of the functions are recursive, but can be easily changed to non-recursive implementation
• Some of the functions have side effects when they unexpectedly (and unnecessary) modify parameters passed by reference
• Optimizations are possible if we lightly modify the behavior (e.g. parameters handling or return values)

Let's look at getElement() as an example:

public static function getElement(&$array, array &$keys, $value = null)
{
    if (count($keys)) {
        $key = array_shift($keys);
        return isset($array[$key])
            ? self::getElement($array[$key], $keys, $value)
            : false;
    }

     if (!is_null($value)) {
        $array = $value;
    }

    return $array;
}

1. The function modifies $array contents, but only if $value is not null.
   Proposal: Drop $value parameter from getElement(). Use setElement() for the only case in Horde_Form_type_image::getUploadedFileType() where the $array is modified.

2. The function modifies $keys array (removes all elements). This is an unneeded side effect, and it looks like existing code does not rely on it anywhere.
   Proposal: Remove & from &$keys.

3. The function is recursive.
   Proposal: Make it non-recursive.

4. In some existing case(s) it would be beneficial to support a case when $keys is a string with a single key.
   For example, in Horde::getDriverConfig($backend, $type = 'sql')):

    if (is_array($backend)) {
        $c = Horde_Array::getElement($conf, $backend);
    } elseif (isset($conf[$backend])) {
        $c = $conf[$backend];
    } else {
        $c = null;
    }

Proposal: Support string in $keys parameter. Additionally, if we modify getElement() to return null instead of false for non-existent properties, then the above snippet could be simplified to just this:

    $c = Horde_Array::getElement($conf, $backend);

And it looks like this will not cause issues with the existing code. Here is one place in Horde_Core_Perms::getAvailable() where the return value is checked for false:

   $children = Horde_Array::getElement($perms['tree'], $levels);
    if (($children === false) ||
        !is_array($children) ||
        !count($children)) {
        /* No array of children available for this permission name. */
        return false;
    }

If returned value becomes null instead of false, it still would still trigger !is_array() check (but we could also adjust the code).

---

With the above proposals the getElement() function transforms into this:

public static function getElement($array, $keys)
{
    $ref = &$array;

    foreach ((array) $keys as $key) {
        if (!isset($ref[$key])) {
            return null;
        }
        $ref = &$ref[$key];
    }

    return $ref;
}

Other functions should be modified in similar way.

[amulet1]

@TDannhauer Let's discuss.

[amulet1]

@TDannhauer Did you see this?

[TDannhauer]

@amulet1 thanks for adressing the reference topic. I stumbled already several times over it. Arguments are passed by reference but not ment to be edited.

I follow your recommendation to clean this corner up. In a lack of time (any maybe knowledge) I'm not able check where we need additional functions to cover the cleanup. Can you take this over for the standard apps including wicked?

[amulet1]

Reopen to be a reminder that the getFieldParts() added by #16 is not yet used anywhere.

Specifically, we want to:

1. Switch to PSR-4.
2. Refactor existing code to rely on getFieldParts() instead of getArrayParts().
3. Recheck and possibly simplify code using getElement()/setElement().
4. Possibly eliminate getArrayParts().
5. Redirect PSR-0 code to PSR-4.

[ralflang]

I don't think we should redirect at all. Build the interfaces we want in src/ with the tools we want. Keep PSR-0 interfaces around for H6 and deprecate them in H6.1 for removal in H7. Don't redirect - this will reduce our freedom to refactor.

[amulet1]

I am in favor of "redirect to identical class => transition to new class => work on new class improvements => drop old class".
Otherwise we will have to maintain 2 versions of classes with similar functionality, with one (newer class) possible receiving incompatible updates.

This, unfortunately, could lead to a situation which we already have in way too many places: migration to new class (or method) was started, but never fully completed for whatever reason, then yet another the new class or method was introduced (or incompatible change was made to the existing class/method), partial migration was then done to the 3rd version, and again never fully completed. And we end up with having several implementations of generally same functionality which are hard to reconcile due to incompatible changes made along the way.

In other words, I am against keeping redundant code except for a very short period of time. If we can not make it quick (and in many cases we can't, if we want to maintain BC), then maybe we should do it in steps:

1. Add new stuff
2. Announce new stuff and migration guidelines [for 3rd parties, if any]
3. Announce (and implement) deprecation of old stuff
4. Migrate to new stuff internally
5. Eventually remove deprecated stuff

(then repeat the cycle)

Because in this specific case we already have:

1. PSR-4 classes compatible with original ones
2. We can use redirectClass() to deprecate old classes.
3. Replace old classes with new names

We have to proceed with migrations to new classes. Once this is completed, we should repeat the cycle, but this time with changes in interfaces.

Making simultaneous migrations to new stuff AND incompatible changes/improvements across large code base split into multiple repositories is a recipe for disaster (well, more likely it would just cause multiple new issues). I am not sure we have enough resources to handle this.

[ralflang]

This is not the way to do it. The wholesale change from the old class names to the new model is a one time situation in which we touch every bit of code. We cannot just freeze development for this activity.

Our migration guide which we followed for quite some time is pretty clear: We keep compatible interfaces around. We keep the old registry ball of mud but don't use it for new code. We keep the old globals-infested factories but don't use them for new code and we wean off them where easily possible. We keep a compatible interface in lib/ by either shims/facades (moderately similar interface), inheritance (rarely, as it couples src) or parallel development (which allows us to strangle out parts into facade classes later).

I'm not going to change that strategy 6-8 weeks before going stable.

After stable, it is exactly as you describe: The lib/ interfaces get a deprecation marker (trait or annotation) and any change to src/ must be non-breaking until we go for FRAMEWORK_7_0.

Alpha phase was exactly about freedom to break otherwise stable designs in src/ when we discover we need to. This phase is coming to an end.

[amulet1]

If we are following this logic, then we should not migrate to src/ until the release is done.

But you are doing just that already (and partial migrations are bad!).

On the contrary, class redirections are 100% safe and could be done at any moment, provided the method names and signatures are compatible.

Alpha phase was exactly about freedom to break otherwise stable designs in src/ when we discover we need to. This phase is coming to an end.

We do not need to break anything. I described more than once how to achieve that. I am all for stability which is still possible with rapid development.

[ralflang]

Partial migrations, strangler patterns, progressive transitions are normal - we cannot and should not expect large transitions to happen in one go. There is always a pilot migration to a new implementation which is thr opportunity to learn some lessons and adapt design. But in Utils it's straight-forward. We want callers to transition to the typed interfaces eventually and at their own speed. We don't want sudden new restrictions in the old interfaces and no log spam. I understand there are scenarios in which language features force bc breaks upon us or the general state of a library demands some minor break & warn such as Forms. But these static helpers are not such a scenario.

I will probably spend some evening documenting this pattern of migration and when we are supposed to log spam and when we really can remove interfaces.