Add option attribute

[nilmerg]

My first prototype contained everything in class Option. Since there may be plans to make more extensive use of attributes, I thought how to generalize their resolution somewhat. Which is why the new interfaces plus the function resolve_attribute are now proposed here as well.

I did not implement the remaining targets (Classes, Parameters, Constants) as they're not needed right now, but the function can be easily extended in the future.

[nilmerg]

Just added another commit, introducing a cache for resolve_attribute(…). Since the reflection api is based on class definitions only, I figured it might be possible to cache most of its results. In practice this means now, that the verification whether an attribute object is valid or not is only done once per class. Class attributes and implements clauses rarely change at runtime, after all. The same is true now for usages of said attribute on other classes, as for them the annotated properties/methods are only inspected once. Attribute instances are cached as well. This has the largest implication, as it allows attributes to track their usage for whatever reason, but also means that there cannot be any user specific state, effectively making them singletons now.

Oh, performance is also increased by 66.67% for a single usage of Option 😛

[lippserd]

I would like to propose some major changes for discussion, as this functionality is generally nothing new, but becomes much more elegant with attributes. Let me first give a general description of how I think the feature should be described, using well-known and established terminology for this functionality, namely hydration:

Implementation of attribute-based object hydration using PHP 8's native attributes for mapping key-value data to object properties and methods. This approach uses the Reflection API in combination with attributes to create a clean, type-safe, and maintainable data mapping system.

Let's now proceed with the classes necessary to provide such functionality, where separation of concerns is crucial. In my opinion, an attribute should not contain any "business logic" but only denote additional "configurations". In line with the topic, I would introduce the components as follows:

AttributeHydrator::hydrate($object, $values), which performs hydration and removes the business logic from Option and the introduced interfaces. As a bonus, this makes it clear that

• It uses PHP 8+ attributes for configuration
• It performs hydration
• It differs from other hydration approaches

Hydrate for the attribute class to
clearly indicate its purpose, i.e., "this property/method is involved in hydration", and its relationship to the AttributeHydrator.

As for the current implementation, I wonder why we should support multiple names and pass values by reference. Do you have a specific use case in mind for this? Why is it necessary? To me, it seems like a "dangerous" combination.

I would also consider introducing mapping strategies for properties/methods to array/data keys, e.g., converting KebapCase/camelCase to snake_case.

When switching to this proposal, most functionality will remain unchanged and are simply transferred to other "owners", where they can then be easily expanded in the future, for example, by enforcing type safety, converting values, etc.

[nilmerg]

If, by business logic, you mean that Option must not be responsible to call a method or set a property, I must assume you also mean, that Option must not have any notion of missing values or alternative names. Leaving Option with nothing, so it's gone.

I don't have a problem with making the function a static method, that indeed won't change anything. But I don't see how removing the business logic from Option doesn't severely impact how it's currently intended.

Yes, I have not described the intention here, but Icinga/ipl-html#157 should cover that. (Except required)

But anyway. Here's a breakdown:

• required: It's mainly a byproduct of a recent refactoring, where I played around with the new Attributes. The alternative would be a getter throwing an exception if nothing is set. The advantage of Option here is that the error occurs earlier, when the form element, decorator, or validator, is created. Not just when it's processed. To me, that fits the intended role of Attributes pretty well.
• name: There are cases of set callbacks in form elements, mapped to differently named setters.
• multiple names: Not important to me, it's just that ipl\Html\FormElement\SelectElement has two set callbacks, one for options and one for multiOptions (zend compatibility), both mapped to the same property/method. I already thought about dropping support multipOptions, so if you don't see a need for this, we can simply do so. Of course, the element can also use two separate setters to set the same property but that makes it part of the public interface.

Stripping down Option leaves us with what? AttributeHydrator::hydrate() looks to me relatively dumb, it mostly maps keys to properties/methods, potentially using a mapping strategy.

Who are the other owners you speak of? Where do they fit in your proposal? How will they be able to do what Option currently does? Please provide an example.

[lippserd]

If, by business logic, you mean that Option must not be responsible to call a method or set a property, I must assume you also mean, that Option must not have any notion of missing values or alternative names. Leaving Option with nothing, so it's gone.

No, I meant that the attribute Option only describes additional metadata, but does not define how it should be processed or applied. That is actually the purpose of attributes:

Attributes enable the decoupling of a feature's implementation from its usage.

Separation of concerns is a must, because otherwise I cannot properly extend, adapt, or reuse the logic, as it would be set in stone. Sure, the code could be rewritten so that Option is extensible, but why should I have to extend an attribute to adapt the logic of how it is applied? Not only does this look strange in terms of code structure, but there is also a good chance that it will become messy.

Stripping down Option leaves us with what? AttributeHydrator::hydrate() looks to me relatively dumb, it mostly maps keys to properties/methods, potentially using a mapping strategy.

Exactly that would be the goal: the attribute describes the metadata and another class uses it. Just compare this approach with the current implementation, where code cluttering interfaces are introduced and the logic is placed in a function that cannot be extended.

Who are the other owners you speak of? Where do they fit in your proposal? How will they be able to do what Option currently does? Please provide an example.

I meant that the code in this PR remains largely unchanged and is simply transferred to other classes, i.e., to the actual hydrator.

However, since you are asking for examples of how this fits into the big picture, consider the proposed Hydrate attribute, which is used for hydrating form data but could also be used for hydrating from database data, e.g., in our ORM. This could also require some additional behavior, i.e., how data is (de)serialized. The AttributeHydrator could then be used with a mapping strategy and serializing logic.

[nilmerg]

I meant that the code in this PR remains largely unchanged and is simply transferred to other classes, i.e., to the actual hydrator.

Okay, let's play that through for a moment:

1. resolve_attribute is the mapping strategy to me, so that's what AttributeHydrator does. It's default strategy is to perform a 1:1 mapping between array keys and properties/setters, with a higher priority of setters over properties
   1. But AttributeHydrator::hydrate($object, $values) does not include the first parameter of resolve_attribute, so how does it know what an Option is?
   2. If it doesn't, it goes over every Attribute and tries to pass the array value to a property or method. Even if the Attribute in question isn't supposed to allow that, take the standard attribute Deprecated for example.
   3. So we need an interface, say AttributeHydration (class Option implements AttributeHydration), that allows AttributeHydrator to do e.g. $property->getAttributes(AttributeHydration, ReflectionAttribute::IS_INSTANCEOF);
2. Option now implements the new interface and only has a constructor left
   1. ::extractValue() does currently two jobs: name mapping, constraint validation
   2. name mapping is a potential candidate for an alternative built-in mapping strategy, as it isn't too uncommon for me to map array keys to differently named properties/setters
      1. But how is this passed to the AttributeHydrator? Maybe using a third parameter for ::hydrate()? (AttributeHydrator::hydrate($object, $values, AttributeMapper $mapper = new PlainAttributeMapper()))
      2. $mapper must be a specific type, let's say a new interface AttributeMapper, which the default strategy already implements: class PlainAttributeMapper implements AttributeMapper
      3. The new strategy is then class DynamicAttributeMapper implements AttributeMapper
   3. constraint validation is somewhat part of the mapping process, so the logical step is to also implement this as mapper?
      1. But there can only be one mapper as of yet, so there must either be a chain of mappers or a simple child of DynamicAttributeMapper (RequiredAttributeMapper extends DynamicAttributeMapper)
   4. Though, honestly, isn't an OptionMapper for both the easier choice?
      1. The DynamicAttributeMapper would need to somehow know of Option::$name which would only be possible by requiring yet another interface (Though, it'd need to require getName() since properties can only be part of interfaces in PHP 8.4)
      2. The same goes for the RequiredAttributeMapper, it would need a new interface for getRequired()

So, to summarize:

Instead of:

• function resolve_attribute
• class Option

We'd have:

• class AttributeHydrator
• interface AttributeHydration
• interface AttributeMapper
• class PlainAttributeMapper
• class DynamicAttributeMapper
• class RequiredAttributeMapper
• interface NamedOption
• interface RequiredOption
• class Option

or alternatively:

• class AttributeHydrator
• interface AttributeHydration
• interface AttributeMapper
• class PlainAttributeMapper
• class OptionMapper
• class Option

I left out the actual implementation of the mappers. That can be dealt with after this.

[lippserd]

I think we should discuss this in person. My impression is that you are thinking the situation is more complicated than it actually is:

<?php

declare(strict_types=1);

#[Attribute(Attribute::TARGET_PROPERTY | Attribute::TARGET_METHOD)]
readonly class Hydrate
{
    public function __construct(
        public ?string $name = null,
        public bool $required = false
    ) {}
}

interface MapperFunc
{
    public function __invoke($tbd): string;
}

/**
 * @template T of object
 */
class AttributeHydrator
{
    /**
     * @param T|class-string<T> $target
     * @param class-string<Hydrate> $attribute
     * @param ?MapperFunc $nameMapper Mapper for name if Hydrate::$name is null.
     * Defaults to SomeDefaultMapper (camelCase to snake_case), pass NoopMapper to explicity disable mapping.
     * Alternatively, let us just require a name in Hydrate. Explicit is better than implicit.
     */
    public function __construct(
        protected readonly object|string $target,
        protected readonly string $attribute = Hydrate::class,
        protected readonly ?MapperFunc $mapper = null
    )
    {
        if (! is_a($attribute, Hydrate::class, true)) {
            throw new ValueError(...);
        }

        // Build cache if necessary. When building the cache,
        // assert that the same name is not used twice.
        // Account mapper in cache.
    }

    /**
     * @return T
     */
    public function hydrate(iterable $values): object
    {
        // If $target is a string, create object dynamically via reflection.
        // If $target is an object, use it directly.
    }
}

[nilmerg]

Maybe, but your pseudo code isn't too far away from what I've described.

• class AttributeHydrator ✔️
• class Hydrate = class Option & interface AttributeHydration
• interface MapperFunc = interface AttributeMapper

But since Hydrate is a readonly class and AttributeHydrator does not accept anything else, I wonder how

where they can then be easily expanded in the future, for example, by enforcing type safety, converting values, etc.

should work. Or at least in which way it'd be different than the current proposal.

I don't see a need for a custom mapper implementation as well anymore, AttributeHydrator only works with Hydrate and so can already provide the required mapping strategy out of the box.

That's what I've tried to avoid. resolve_attribute is a generic implementation to reflect attributes on properties and methods, but doesn't mandate how they're being interpreted. This is up to the attribute in question. (i.e. Option)

If we'd implement the AttributeHydrator as you describe above, any attribute implementation in the future will have to handle reflection and caching on its own again. If that's what we want, fine. But to me that's at least one flaw of PHP's attribute system.