[RFC]: Hybrid options transform system

[codisart]

RFC

Q	A
Proposed Version(s)	2.15.x
BC Break?	No

Goal

Remove duplication and simplify for the initialisations of the option system.

Background

Several validator objects use an hybrid system of options.
It can be passed as an single array argument or as multiples arguments of the constructor
So a part of the job of the constructor is to transform options into an array.
And several validators do this job and it is not easy to understand and to read.

Considerations

I expect a good impact on maintainers due to this change.
There should be no impact on current behavior.

Proposal(s)

The proposal is to use a class with a static factory method to init the options array
This method will accept:
as first argument an array of the options keys that always should be string
as second argument an array that should always be the array returned by the function func_get_args()

$options = ValidatorOptions::asArray(['min', 'max', 'inclusive'], func_get_args());

class ValidatorOptions
{
    public static asArray(array $keys, array $args) {
        $this->shouldHaveOnlyStringValues($keys);

        $result = [];
        foreach ($args as $arg) {
            $result[array_shift($keys)] = $arg;
        }
        return $result;
    }

    private function shouldHaveOnlyStringValues(array $keys) : void
    {
        foreach ($keys as $key) {
            if (! is_string($key)) {
                throw new \InvalidArgumentException('The values of $keys should be only strings');
            }
        }
    }
}

Appendix

I'd like to thank @glensc for the final form of the proposal

Here the PR with the two first forms of the proposed solution
#60

[gsteel]

Interesting idea, but I personally would prefer to see only array shapes for options, obviously as a BC breaking change for the next major.

A lot of the validators accept an options array or a specific option as argument 1, such as Between which is effecively Between::__construct(array|int $minOrOptions, int|null $max, bool|null $inclusive). This is difficult to document and maintain and I'd much rather see this:

/**
 * @psalm-type Options = array{
 *    option1: non-empty-string,
 *    option2: int<1, 42>,
 *    optional?: bool,
 * }
 */
final class SomeValidator implements ValidatorInterface
{
    /** @param Options $options */
    public function __construct(private array $options)
    {
    }
}

I'd also be in favour of removing getOptions, setOptions, all setters and getters for individual options, and the idea that options arrays can be Traversables too.

[codisart]

@gsteel I agree completely with your specification for the next major.

I tried to find a way to make the code for options more comprehensible without BC.
Maybe this could be implemented before the next major.

[boesing]

If it comes to me, I'd rather remove arrays from __construct at all and instead implement dedicated properties instead.
Some validators might have required options and won't work without these. Even tho that could be fixed with array shapes, I would prefer native typed constructor arguments over array shapes as it becomes way more readable for manually instantiated validators. For configurations such as input-filter, etc., a dedicated factory can be registered (or maybe a generic factory) which then extracts all options from the array $options factory argument.

I wrote this code for monolog handler/formatter instantiation some time ago:

final class Instantiator implements InstantiatorInterface
{
    /**
     * Handles the constructor arguments and if they're named, just sort them to fit constructor ordering.
     *
     * @template T
     * @psalm-param class-string<T> $className
     *
     * @return T
     * @throws InvalidArgumentException if given arguments are not valid for provided className constructor
     */
    public function createInstanceWithArguments(string $className, array $arguments): object
    {
        $reflection = new ReflectionClass($className);
        $constructor = $reflection->getConstructor();

        // There is no or at least a non-accessible constructor for provided class name,
        // therefore there is no need to handle arguments anyway
        if ($constructor === null) {
            Assert::count($arguments, 0, sprintf(
                '%s has configured %%2$d arguments but does not have any constructor!',
                $className
            ));

            return $reflection->newInstance();
        }

        if (!$constructor->isPublic()) {
            throw new InvalidArgumentException(sprintf(
                '%s::__construct is not public and therefore it is not instantiable',
                $className
            ));
        }

        $requiredArgsCount = $constructor->getNumberOfRequiredParameters();
        $argumentCount = count($arguments);

        if ($requiredArgsCount > $argumentCount) {
            throw new InvalidArgumentException(sprintf(
                '%s::__construct() requires at least %d arguments; %d given',
                $className,
                $requiredArgsCount,
                $argumentCount
            ));
        }

        // Arguments supposed to be ordered
        if (isset($arguments[0])) {
            return $reflection->newInstanceArgs(array_values($arguments));
        }

        $parametersOrderedByPosition = [];

        foreach ($constructor->getParameters() as $parameter) {
            $parameterName = $parameter->getName();

            if (array_key_exists($parameterName, $arguments)) {
                $parametersOrderedByPosition[$parameter->getPosition()] = $arguments[$parameterName];

                continue;
            }

            if (!$parameter->isOptional()) {
                throw new InvalidArgumentException(sprintf(
                    'Missing at least one required parameters `%s`',
                    $parameterName
                ));
            }

            $parametersOrderedByPosition[$parameter->getPosition()] = $parameter->getDefaultValue();
        }

        return $reflection->newInstanceArgs(array_values($parametersOrderedByPosition));
    }
}

That could be somehow used for a generic factory to actually instantiate validators with proper constructors rather than having array shapes. But maybe thats just my personal preference, but I highly dislike these array shapes since like forever.

---

These are just my two cents, I do not expect this PR to be rewritten. Just wanted to share my idea on this as I was thinking about this for laminas-cache as well, since there are already adapter options, I would most probably add proper arguments to those classes anyways and ban AdapterOptions at all (which is a "generic" options class used in adapters without dedicated options).

[gsteel]

My hope is that we can get a v3 of validator out the door so that we can use service manager v4 and therefore psr/container v2, which is long overdue.

Validator has truck loads of legacy that needs cleaning up, and like @boesing, I'd be much happier with factories and explicit, typed, constructor args, or well-known array shapes for options… so, I'd rather see effort target the next major if someone has the time and energy for it, but I certainly wouldn't reject any valid improvement of the current situation.

If you can improve the options situation whilst preserving BC, and improvents to 2.x are important to you then I'd just be grateful :)

I'll take a look at the existing patch asap 👍

[Xerkus]

We had this via laminas-stdlib AbstractOptions. This approach was removed from components over the years for better or for worse.
I would say this proposal is a no go.