fix: Correct Horde_Form_Type::isValid() signature

[amulet1]

As far as I understand, by default (without &) PHP passes non-objects (which includes arrays) by value.

For example:

<?php
function test($info, $message) {
        $info['test'] = 123;
        $message = 'test';
}

$info = [ 'test' => 222 ];
$message = 'message';
print "test=" . $info['test'] . " message=$message\n";

test($info, $message);

print "test=" . $info['test'] . " message=$message\n";

Output (nothing changes):

test=222 message=message
test=222 message=message

The commit 070af29 removed & from $message in IsValid() signature. This causes loss of value possibly assigned to $message.

Possible example of other signature issue (not in this patch, but I can update it, if needed) is in getInfo. Implementations are inconsistent - some return $info, others modify its in place.

But in the below case nothing is returned and $info is not passed by reference, so the assigned value is lost:

class Horde_Form_Type_stringarray extends Horde_Form_Type_stringlist {
    function getInfo($vars, $var, $info)
    {
        $info = array_map('trim', explode(',', $vars->get($var->getVarName())));
    }

If this PR is accepted, signatures in other apps (for example, see Ingo_Form_Type_Longemail::isValid()) that extend Horde_Form_Type or its derivatives need to be updated as well.

[ralflang]

I'm not so fond of this approach. Alternatives:

• Store a property $invalidMessage either with a read hook (PHP 8.4+ only) or with a getter (universally possible)
• Emit a rich object if valid.

As for the getInfo, we have made virtually all getInfo usages return the value and read the return value. Let me look up any leftovers.

[amulet1]

In this case $message parameter should be dropped altogether.

And all the callers should be updated to retrieve the property, if it is needed.

getInfo still needs to be fixed ($infoparameter should be dropped too).

I would like to offer my help with the changes. Please advise.

[ralflang]

Help is much appreciated. However we should be careful with timing.

Changing signatures again in a base class which is littered all over horde and external products needs a bit of care. We want to go beta next week and stable in July. Let's work on Forms 4 with a proper OO model and then upgrade apps step by step in a .1 feature release.

[amulet1]

Well, right now it is broken - $message is not returned at all. It has to be fixed.

Maybe we can do it in stages:

1. Make $message parameter optional. Potentially log warning/stack trace if the argument count includes the optional parameter.
2. Eliminate it on the caller's end, update code to retrieve $message property where needed. This can take some time.
3. Finally remove $message parameter.

Same can be done with getInfo. Not sure if there are other places like that, as I just looked at the very recent commits that were not working for me.

[ralflang]

Yes, let's do it in stages.

• Callers may upgrade right now:

// old:
$form->getInfo($vars, $info)
$type->getInfo($vars, $info);
$bValid = $type->isValid($var, $vars, $value, $message);

// new:
$info = $form->getInfo($vars);
$info = $type->getInfo($vars, $info);
$bValid = $type->isValid($var, $vars, $value, $message);
$message = $type->getMessage();

Let's implement the incompatible, better type system in the PSR-4 namespace (src folder)

src/V3/Types/Text.php

$info = $form->getInfo($vars);
$info = $type->getInfo($vars);
$bValid = $type->isValid($var, $vars, $value);
$message = $type->getMessage();

This way

• caller code than upgrade to TypesV3 at their own pace
• There is no hard break. Most code I tested is already upgraded for the crucial getInfo functionality. The $message functionality can be updated one library/app at a time either to just add the getMessage line (right now) or to use the new signature (as soon as we have that)
• All progressively possible before July i.e. before the Horde 6 mainline release or after. No time pressure.
• A more involved V4 redesign can coexist.

[ralflang]

Superseded by #6 - Let's do it in stages.