Re: [Perl-workflow-devel] [rt.cpan.org #38024] all conditions with 'test' attirbute appears to be cached as 'evaluate'

[Jim B. <cb...@bu...>]

I'm ok with the principle of the patch. The special 'test' conditions 
are conditions with raw Perl code rather than an explicit condition 
object. The _create_condition_objects sub then creates a condition 
object on the fly for each of these. The problem is the name of each 
auto-generated condition object is hard-coded to 'evaluate'.

I would want to verify if the 'evaluate' name is a magic name used 
somewhere else in the code such that it expects it to be hard coded that 
way.

Also, the patch uses the value of the test attribute as the name. While 
this would guarantee that it will be unique, it could be an arbitrarily 
large hash key (you can put in as much perl code as you want) and it 
wouldn't be very human readable if you were ever debugging.

I would suggest adding some code to name it something like:

"_" . <action_name> . "_condition_" . $counter

where action_name is the name of the enclosing action and the counter is 
to allow us to have multiple 'test' type conditions for an action. The 
leading underscore is just to indicate that is is private 
(auto-generated). So if you were debugging, the names of these condition 
objects would look something like:

_MyAction_condition_1

I work with Bob (Robert), so we'll discuss in the next day or two.

Jim


Jonas Brømsø Nielsen wrote:
> Hello All,
> 
> I have not received any feedback and I am reluctant to making a
> development release for a one-line patch, I was hoping we would be
> able to evaluate this and decide simply on the contents of the patch.
> 
> The issue is described below and the patch goes for line: 349 in HEAD
> 
> The line:
>         name  => 'evaluate',
> 
> Roberts wants changed to:
> 
>         name => "_$condition_info->{test}",
> 
> I have not really gotten my head around the condition objects (I
> should do this soon since our docs needs a serious update).
> 
> But this comment just above makes me hesitate applying the patch:
> 
>         # Special case: a 'test' denotes our 'evaluate' condition
> 
> So I guess the evaluate name is supposed to be hard-coded. However
> Roberts patch does seem to offer some flexibility. But then the _
> (underscore) troubles me.
> 
> I am going to go over the tests covering this part later tonight, but
> if anybody has suggestions or opinions on this please feel free to
> write me (the list). Jim, I am particularly interested in your
> opinion :)
> 
> Now I will go for my run to clear my head then I will look at the code.
> 
> 
> jonasbn
> 
> On 24/10/2008, at 23.55, Jonas Brømsø Nielsen wrote:
> 
>> Hiya Alll,
>>
>> We have received a patch from Robert Stockdale.
>>
>> If anybody has time to evaluate it, please feel free.
>>
>> jonasbn
>>
>> Begin forwarded message:
>>
>>> From: "Robert Stockdale via RT" <bug...@rt...>
>>> Date: 29. sep 2008 21.48.42 GMT+02:00
>>> To: undisclosed-recipients:;
>>> Subject: [rt.cpan.org #38024] all conditions with 'test' attirbute
>>> appears to be cached as 'evaluate'
>>> Reply-To: bug...@rt...
>>>
>>>      Queue: Workflow
>>> Ticket <URL: http://rt.cpan.org/Ticket/Display.html?id=38024 >
>>>
>>> Hi Jonas,
>>>
>>> Sorry for the delay. I was able to confirm this bug.
>>>
>>> The name is being explicitly set to 'evaluate' in
>>> Workflow::State::_create_condition_objects for all conditions that
>>> specify 'test' as an attribute in the xml configuration. This,
>>> coupled
>>> along with caching of condition results means that only the first
>>> 'evaluate' condition will actually be evaluated and each subsequent
>>> call
>>> will use the cached result.
>>>
>>> Attached is a patch that will resolve the issue. Essentially I am
>>> just
>>> taking the value of the 'test' attribute and naming the condition
>>> that,
>>> with one small exception. Perl seems to have some issue with keys
>>> that
>>> start with '!' so I prefixed it with an underscore. I tested this
>>> change
>>> and it is working in my dev environment.
>>>
>>> An alternative option would be to allow the user to specify the
>>> 'name'
>>> attribute in the xml just like with any other condition. I did not
>>> exercise this option.
>>>
>>> -Bob
>> <evaluate_condition_name.patch>
> 

-- 
Jim Brandt
Administrative Computing Services
University at Buffalo