WIP: Fix callback inconsistency

[lippserd]

Attribute callbacks should work with all methods in the Attributes class. Right now they are only evaluated in render(). Further, there is no error handling if a callback fails. This PR attempts to fix the following issues:

1. Attributes::get() must return an Attribute instance which is produced from the callback's result. If the callback fails, a RuntimeException is raised. If the callback's result type is invalid, a UnexpectedValueException is raised. The exception stuff applies to render() as well.

2. Setter callbacks must be proxied in the Attribute instance returned from a callback in Attributes::get().

3. Attributes::set() and Attributes::add() must use registered callbacks. If the attribute already exists in add() an exception must be raised.

4. If there is no setter callback, an Exception must be raised when trying to override the respective attribute.

5. When trying to add callbacks to an attribute which already exists, an exception is raised.

6. Attributes::get() must raise an exception if there is only a setter callback specified.

For the moment I just added failing tests for the uses cases above. @Thomas-Gelf Please evaluate whether this makes sense. I'll start fixing this then.

[Thomas-Gelf]

We should settle on a single name, I already renamed this once for you ;-) We could also introduce different Setters for Read/Write Callbacks. Because with this signature, I'm unsure what would happen when running:

->setCallback('value', null, [$this, 'setValue'])->setCallback('value', [$this, 'getValue'])

I would expect my setter to be still in place. Do you have a good proposal for this?

And apart from this, I didn't grasp what inconsistency this is going to fix, this is just a proxy method, isn't it? Is it about naming?

[lippserd]

It's not about the method name or signature. But I'd go with the following methods:

• registerCallbacks($attribute, $getter, $setter)
• registerGetter($attribute, $getter)
• registerSetter($attribute, $setter)

The inconsistency is about the following (not complete, just to give you an idea):

1) Attributes::get() has no clue about callbacks:

Example 1: Getter callback for the attribute class.

$attrs = new Attributes();

$attrs->registerGetter('class', function () {
    return new Attribute('class', 'state-critical');
});

$attrs->get('class')->getValue(); // returns null

Example 2: Setter callback for the attribute class:

$attrs = new Attributes();

$attrs->registerSetter('class', function ($value) {
    // Magic happens here
});

$attrs->get('class'); // Must throw an exception because there is no getter for class

Example 1: Getter callback return values must be validated:

$attrs = new Attributes();

$attrs->registerGetter('class', function () {
    return new Attribute('cheat', 'state-critical');
});

$attrs->get('class'); // Must throw an exception because the attribute's name from the callback does not match the registered name

2) Attributes::set() is missing callback validation:

Example 1: Getter callback for the attribute class.

$attrs = new Attributes();

$attrs->registerGetter('class', function () {
    return new Attribute('class', 'state-critical');
});

$attrs->set('class', 'state-ok'); // Must throw an exception because there is no setter for class

Example 2: Ambiguous state when using Attribute instances in Attribute::set():

$class= 'state-ok';

$attrs->registerGetter('class', function () use ($class) {
    return $class;
});

$attrs->registerSetter('class', function ($value) use (&$class) {
    $class = $value;
});

$attrs->set(Attribute::create('class', 'state-critical'));

echo $class; // Yields 'state-ok' though it should 'state-critical'

3) The methods for registering callbacks must throw exceptions if the attribute already exists:

$attrs = new Attributes(['class' => 'state-ok']);

// Exception must be thrown because the attribute already exists
$attrs->registerGetter('class', function () {
});

4) Attributes from getter callbacks should have the setter callback proxied?

$attrs = new Attributes();

$class = Attribute::create('class', 'state-ok');

$attrs->registerGetter('class', function () use ($class) {
    return $class;
});

$attrs->registerSetter('class', function ($value) use ($class) {
    $class->setValue($value);
});

$fromCallback = $attrs->get('class');
$fromCallback->setValue('state-critical');

echo $fromCallback->getValue(); // Yields state-critical
echo $class->getValue(); // Yields state-ok though it should state-critical too

This is a little bit of a headache because we would need callback handling in the Attribute class too.
I'd really like to see another approach how to handle this.

I'd be happy to fix this.

[Thomas-Gelf]

First of all, Getter and Setter Callbacks are to be considered complete distinct beasts. The fact that there is a Getter should in no way have any influence on what to expect when getting an Attribute.

There are different use-cases, sometimes you want to "steal" a value, work with it but never set a related attribute. Therefore I do not like the idea of proxying setters in getted values. I understand the technical explanation, but see no use in it.

So, IMO:

1. -> that's true, could be fixed, I don't really care - that's not how I intended to use it
2. -> no
3. -> no, could be legal/handled. Related: what happens when you register the the same getter twice?
4. -> no

Of course I'm open for arguments pointing to the other direction. Is this pull request here to address a just the inconsistency, or do you have related use-cases you want to see addressed?

Thanks,
Thomas

[lippserd]

I want to address the inconsistency. Related use cases are in the failing tests and the examples I gave.

First of all, Getter and Setter Callbacks are to be considered complete distinct beasts.

That's the whole problem I'd like to fix. Callbacks must be seen as attributes else we face ambiguous states.

The fact that there is a Getter should in no way have any influence on what to expect when getting an Attribute.

Why should it? Why do I have to expect different results from render() and get()?

There are different use-cases, sometimes you want to "steal" a value, work with it but never set a related attribute.

I saw that in the form implementation. IMO that's flawed. Attributes hold HTML5 attributes and should not be abused for class configurations. But that's a completely different story which is not part of this issue.

With the current implementation you may build attributes with completely different states in the very same instance. Why do even think about not fixing this?

Back to the points:

2. Why not? The whole point of the Attributes class is to render HTML5 attributes. Why is get() and set() different to render() then?

3. What is the point of having more than one getter? Here you see another problem of this callback stuff. In our forms we rely on having attribute callbacks. What happens if I override them? Shouldn't callbacks lock an attribute? I really do think that we have protect us and other developers from this potential mess.

4. It is possible to mess with this, so it has to be fixed in one way or another.

TL;DR

We should settle on the following rules for callbacks:

1. Callbacks should be used internally only to proxy attribute manipulation in classes which use Attributes.
2. Callbacks "lock" their respective attributes.
3. If there's only a getter callback, it is impossible to set the attribute. No matter which signature I use.
4. If there's only a setter callback, it is impossible to get the attribute. No matter which signature I use. Though I really discourage this.
5. I can't override callbacks.

Isn't that exactly your use case of callbacks? You want to manipulate how the value attribute is handled for example and must rely on the fact that nobody is able to change this.

[Thomas-Gelf]

I want to address the inconsistency. Related use cases are in the failing tests and the examples I gave.

Real world use-cases would be great. I understand the inconsistency problem, I'm trying to evaluate whether it is worth to increase complexity while making it harder to implement the use-cases this has been built for.

Callbacks must be seen as attributes else we face ambiguous states.

That's where I disagree, those callbacks have initially been introduced to fake state and behavior for various reasons.

Why do I have to expect different results from render() and get()?

That's what this feature is all about. get() behaving different is a side-effect, I do not care at all. Wherever a setter is in place there is usually a dedicated getter to retrieve the intercepted property. I fear that when going this way we might to be forced to separate form options from form element attributes. That's a path that ZF2 has taken, it didn't make things easier.

As of now, a SelectElement for example intercepts the "options" attribute. I do not expect to get an Attribute with a dictionary value when running Attributes->get('options'). That wouldn't even be valid. The main rule is that options not consumed be the FormElement are HTML attributes. That's just for convenience, for serialization, for easy form setup. Keeping the creation and serialization of elements easy it the main use-case for those Callbacks.

A SubFormElement could be populated with an 'value' property, but that's not an Attribute at all - it just populates other elements. Pick the submit label, it renders a value attribute, but that attributes value is the label(!). Still, set for the fake value attribute must remain in place. And getValue is then used to determine whether it has been pressed.

This hooks allow us to cheat. Such cheats give a powerful instrument and allow to avoid some pitfalls ZF1 ran into. When you try to make it consistent, you risk to break the possibility to cheat. And by the end of the day, that's what this is all about. When I need an Attribute with special behavior I can extend the Attribute class, no need for a hook - that's a completely different use-case.

Do you understand what I mean?

[nilmerg]

I stumbled over this today while implementing the datetime-local input. This hides the string representation from the user and only handles DateTime objects in getValue() and setValue(). Though, its callbacks are ignored when still wanting to access the actual string value by use of getAttributes()->get()->getValue().

[sukhwinder33445]

While working on FormElement: Add RadioElement #68 PR and Enhance select element #88, I noticed that the attributes need to be in a certain order to work properly. (similar problem).

Currently I have fixed this as follows:

ipl-html/src/FormElement/SelectElement.php

Line 33 in e71d5f3

if (is_array($attributes)) { // because attributes can be in any order

However, there may be other attributes that should be in a certain order.

It would be nice if we could fix this dynamically, without having to sort the attribute in the element's constructor.

[lippserd]

The particular order is only necessary because the code is flawed. There must be no dependencies between setters. Although one could argue that the value should simply always be set last. But even that can easily be bypassed by calling the setters instead of using the construct-time attributes:

(new FlawedElement())->setValue('bypassed')->setAttribute('attribute-that-setValue-depends-on', 'value');

Sure, there could be an attribute callback that adjusts a value that's already set, but honestly, ... 🙅

[nilmerg]

Didn't read over this all over again, but I'd like to add this:

1. I have a form
2. It validates an input using a callbackvalidator
3. This validator may decide that a part of the value is valid and another part is not
4. The invalid part should remain in the input, the valid part is processed elsewhere (note: if everything is valid, the input should be empty)
5. Since Don't cache element validation state #97 this breaks the form's validation state as the element's value is changed
6. My fix for this is to only set the value of the rendered attribute. So, $element->getValue() still returns the submitted value, $element->getAttributes()->get('value') does not
7. This is currently only possible to achieve by doing this: $element->getAttributes()->registerAttributeCallback('value', function () { return ''; })
8. My attempt before this was: $element->getAttributes()->setAttribute(Attribute::create('value', '')) which should have reset the attribute's callbacks IMHO (didn't)
9. My first attempt was: $element->getAttributes()->set('value', '') which didn't work (correct! IMHO) as the setter callback got called