[#11124] decode_attribute hook an exceptions
Summary decode_attribute hook an exceptions
Queue Turba
Queue Version 3.0.12
Type Enhancement
State Feedback
Priority 1. Low
Owners
Requester kd@tu-cottbus.de
Created 2012-04-02 (3055 days ago)
Due
Updated 2012-04-04 (3053 days ago)
Assigned
Resolved
Milestone
Patch No

Comments
kd@tu-cottbus.de 2012-04-02 08:54:49
playing with a decode_attribute hook, i noticed, that it only works correctly,
if for each and every attribute in every source that is not handled by 
the hook,
an exception must be thrown
(at least composite fields and images are affected).
In real life only a few attributes from one source need conversion.
One possible solution would be a change in the logic of the getValue method
in Object.php (move the Hook call near the end).


Jan Schneider <jan@horde.org> 2012-04-02 21:57:36
The purpose of the hook *is* to be called for any attribute. I don't 
understand what you trying to achieve, besides that it would break the 
intended behavior.

kd@tu-cottbus.de 2012-04-03 06:00:51
> The purpose of the hook *is* to be called for any attribute. I don't 
> understand what you trying to achieve, besides that it would break 
> the intended behavior.
hmm,
i have a personal adressbook 'localsql' (nothing special, mainly the 
sample code) and
a global LDAP adressbook 'localldap'. Now i have no field for e.g. ' 
freebusyUrl' in LDAP.
So i started with:
class Turba_Hooks
{
      public function decode_attribute($attribute, $value, $contact)
      {
            switch ($attribute) {
            case 'freebusyUrl':
                   ....
                   return $url;
            }
           return $value;
      }
}
Now I have 'freebusyUrl' in 'localldap', but magically lost all 
pictures in 'localsql'.
To get  the pictures in 'localsql' back, i have to throw an exception 
instead of the
default passthrough 'return $value'.
Now it works as expected, but i have tons of "No decode handler" 
errors in the Logs.
What I'm doing wrong here?

Jan Schneider <jan@horde.org> 2012-04-03 08:32:12
> To get  the pictures in 'localsql' back, i have to throw an 
> exception instead of the
> default passthrough 'return $value'.

Yes, because that's how we detect if an attribute was *not* processed 
by a hook, and thus continue the further processing of composite and 
image fields further down in getValue(). If you moved the hook calling 
further down, you wouldn't be able to hook into those fields at all.

> Now it works as expected, but i have tons of "No decode handler" 
> errors in the Logs.
> What I'm doing wrong here?

Nothing, though I admit that having the logs spammed with false 
warnings isn't optimal.

Jan Schneider <jan@horde.org> 2012-04-03 08:37:12
We may want to introduce a new Exception class that is thrown to skip 
hook calls and that's not being logged in Horde::callHook().
It's still a performance degradation to throw and catch so many 
exceptions, but I don't see an alternative to reliably skip processing 
of individual attributes. Well, maybe we could return an array in 
Turba 4 instead, with a boolean as the first element, whether the hook 
has been called.

kd@tu-cottbus.de 2012-04-03 11:56:47
>> To get  the pictures in 'localsql' back, i have to throw an exception
>> instead of the
>> default passthrough 'return $value'.
>
> Yes, because that's how we detect if an attribute was *not* 
> processed by a hook, and thus continue the further processing of 
> composite and image fields further down in getValue(). If you moved 
> the hook calling further down, you wouldn't be able to hook into 
> those fields at all.
>
>> Now it works as expected, but i have tons of "No decode handler"
>> errors in the Logs.
>> What I'm doing wrong here?
>
> Nothing, though I admit that having the logs spammed with false 
> warnings isn't optimal.

Just a few thoughts:

- to detect, whether or not an attribute has processed, one could
compare the value before and after the hook
- is it really desired, to have composite fields processed by the hook?
They are a combination of  driver values (that should be processed)
- are images in this sense not some special kind of composite fields?



Jan Schneider <jan@horde.org> 2012-04-03 12:23:29
>>> To get  the pictures in 'localsql' back, i have to throw an exception
>>> instead of the
>>> default passthrough 'return $value'.
>>
>> Yes, because that's how we detect if an attribute was *not* processed
>> by a hook, and thus continue the further processing of composite and
>> image fields further down in getValue(). If you moved the hook
>> calling further down, you wouldn't be able to hook into those fields
>> at all.
>>
>>> Now it works as expected, but i have tons of "No decode handler"
>>> errors in the Logs.
>>> What I'm doing wrong here?
>>
>> Nothing, though I admit that having the logs spammed with false
>> warnings isn't optimal.
>
> Just a few thoughts:
>
> - to detect, whether or not an attribute has processed, one could
> compare the value before and after the hook

Not easily possible, because the values aren't necessarily scalar values.

> - is it really desired, to have composite fields processed by the hook?
> They are a combination of  driver values (that should be processed)

This is debatable, but I don't see a reason to artificially limit 
which fields we allow to pass through the hook, just for code-flow 
reasons.

> - are images in this sense not some special kind of composite fields?

Not really.

kd@tu-cottbus.de 2012-04-03 20:07:38

> This is debatable, but I don't see a reason to artificially limit 
> which fields we allow to pass through the hook, just for code-flow 
> reasons.
>
A composite field is defined in terms of "turba-attributes" (left-hand 
side of the map),
this is different from directly  to "backend attributes" mapped 
"turba-attributes".
What is passed to the hook as the value of a composite field?
I can't imagine a situation, where a composite field *and* a hook for 
that field makes sense.
I'd always use a direct mapping and do the other work in the hook.
Isn't a composite field not some kind of simple inline hook?

> Well, maybe we could return an array in Turba 4 instead, with a 
> boolean as the first element, whether the hook has been called.
Is it possible to do it the old-fashioned way,  pass the value by 
reference, and return a
status code?


Jan Schneider <jan@horde.org> 2012-04-03 20:42:10
>
>> This is debatable, but I don't see a reason to artificially limit
>> which fields we allow to pass through the hook, just for code-flow
>> reasons.
>>
> A composite field is defined in terms of "turba-attributes" 
> (left-hand side of the map),
> this is different from directly  to "backend attributes" mapped 
> "turba-attributes".
> What is passed to the hook as the value of a composite field?
> I can't imagine a situation, where a composite field *and* a hook 
> for that field makes sense.
> I'd always use a direct mapping and do the other work in the hook.
> Isn't a composite field not some kind of simple inline hook?

I can imagine a field that requires different inputs from the user, 
i.e. different attributes, but the result should be rendered in a 
single composite field, after passed through the decode hook.

>> Well, maybe we could return an array in
>> Turba 4 instead, with a boolean as the first element, whether the hook
>> has been called.
> Is it possible to do it the old-fashioned way,  pass the value by 
> reference, and return a
> status code?

No, for that to work, the arguments need to passed by reference to 
Horde::callHook() too.

kd@tu-cottbus.de 2012-04-04 08:13:21

> I can imagine a field that requires different inputs from the user, 
> i.e. different attributes, but the result should be rendered in a 
> single composite field, after passed through the decode hook.
>
Just for completeness, can you be a bit more specific, i don't understand,
why the composite field is essential here.

For now i have no new arguments/ideas. Returning an array is ok.

Btw. i havn't had a closer look at the opposite side (setValue/encode),
composite fields seem to be a special case here too (not editable in
the contact form)

kd@tu-cottbus.de 2012-04-04 13:41:56
Yet another try:

Change the logic in getValue:
if composite filed - compose it, (don't return)
if hook is defined - call it (don't return)
if image - wrap it into array
return