Mailing List Archive

On Fri, 6 May 2016, Mike Heins wrote:

> Quoting Sam Batschelet (interchange-cvs@icdevgroup.org):
>> commit 8ba9a07d0f0098b5cae6cb4400bf8ead5f4aa91b
>> Author: Sam Batschelet <samb@endpoint.com>
>> Date: Thu May 5 11:09:09 2016 -0400
>>
>> Fix for restrict_allow which was being clobbered by default values.
>>
>> lib/Vend/Table/Editor.pm | 6 +++++-
>> 1 files changed, 5 insertions(+), 1 deletions(-)
>> ---
>> diff --git a/lib/Vend/Table/Editor.pm b/lib/Vend/Table/Editor.pm
>> index 141eed3..de9eb7a 100644
>> --- a/lib/Vend/Table/Editor.pm
>> +++ b/lib/Vend/Table/Editor.pm
>> @@ -916,7 +916,11 @@ sub display {
>> last METAMAKE;
>> }
>>
>> - $opt->{restrict_allow} ||= $record->{restrict_allow};
>> + if ($record->{restrict_allow}) {
>> + my %restrict_allow;
>> + @restrict_allow{grep /\S/, split / /, $opt->{restrict_allow} . ' ' . $record->{restrict_allow}} = ();
>> + $opt->{restrict_allow} = join ' ', sort keys %restrict_allow;
>> + }
>> #::logDebug("formatting prepend/append/lookup_query name=$opt->{name} restrict_allow=$opt->{restrict_allow}");
>> for(qw/append prepend lookup_query/) {
>> next unless $record->{$_};
>>
>
> Doesn't this prevent you from passing a restrict_allow option?

It merges the two sets of restrict_allow values together.

Jon

--
Jon Jensen
End Point Corporation
https://www.endpoint.com/

_______________________________________________
interchange-users mailing list
interchange-users@icdevgroup.org
http://www.icdevgroup.org/mailman/listinfo/interchange-users

Quoting Jon Jensen (jon@endpoint.com):
> On Fri, 6 May 2016, Mike Heins wrote:
>
> >Quoting Sam Batschelet (interchange-cvs@icdevgroup.org):
> >>commit 8ba9a07d0f0098b5cae6cb4400bf8ead5f4aa91b
> >>Author: Sam Batschelet <samb@endpoint.com>
> >>Date: Thu May 5 11:09:09 2016 -0400
> >>
> >> Fix for restrict_allow which was being clobbered by default values.
> >>
> >> lib/Vend/Table/Editor.pm | 6 +++++-
> >> 1 files changed, 5 insertions(+), 1 deletions(-)
> >>---
> >>diff --git a/lib/Vend/Table/Editor.pm b/lib/Vend/Table/Editor.pm
> >>index 141eed3..de9eb7a 100644
> >>--- a/lib/Vend/Table/Editor.pm
> >>+++ b/lib/Vend/Table/Editor.pm
> >>@@ -916,7 +916,11 @@ sub display {
> >> last METAMAKE;
> >> }
> >>
> >>- $opt->{restrict_allow} ||= $record->{restrict_allow};
> >>+ if ($record->{restrict_allow}) {
> >>+ my %restrict_allow;
> >>+ @restrict_allow{grep /\S/, split / /, $opt->{restrict_allow} . ' ' . $record->{restrict_allow}} = ();
> >>+ $opt->{restrict_allow} = join ' ', sort keys %restrict_allow;
> >>+ }
> >> #::logDebug("formatting prepend/append/lookup_query name=$opt->{name} restrict_allow=$opt->{restrict_allow}");
> >> for(qw/append prepend lookup_query/) {
> >> next unless $record->{$_};
> >>
> >
> >Doesn't this prevent you from passing a restrict_allow option?
>
> It merges the two sets of restrict_allow values together.

Hmm. I don't understand the comment then, "was being clobbered by
default values". This more looks like a fix for the default values
getting clobbered by the $opt, which I would think is the intention
of the code.

In other words, if you set it in $opt it should blow away what's in
$record, as presumably your setting of the option should indicate that
you know what's best. .

--
Mike Heins
End Point -- Expert Internet Consulting http://www.endpoint.com/
phone +1.765.253.4194 <mikeh@endpoint.com>

"All you need in this life is ignorance and confidence, and
then success is sure." -- Mark Twain

_______________________________________________
interchange-users mailing list
interchange-users@icdevgroup.org
http://www.icdevgroup.org/mailman/listinfo/interchange-users

On 05/06/2016 05:23 PM, Mike Heins wrote:
> Quoting Jon Jensen (jon@endpoint.com):
>> On Fri, 6 May 2016, Mike Heins wrote:
>>
>> It merges the two sets of restrict_allow values together.
>
> Hmm. I don't understand the comment then, "was being clobbered by
> default values". This more looks like a fix for the default values
> getting clobbered by the $opt, which I would think is the intention
> of the code.
>
> In other words, if you set it in $opt it should blow away what's in
> $record, as presumably your setting of the option should indicate that
> you know what's best. .

Mike,
My understanding is $opt->{restrict_allow} was defined by the
o_default_defined hash or $CGI->{restrict_allow} values. And as $opt
was always defined, if you set restrict_allow via meta_editor,
$record->{restrict_allow} would never be set by this line.

$opt->{restrict_allow} ||= $record->{restrict_allow};

So perhaps the word clobbered was dramatic, but you could not define
$opt even if it were only the default values with $record. As Jon said
the patch takes the $opt values and merges them with $record. This way
you can maintain defaults as well as possible input options. Certainly
open to better way to handle this. The commit message would read better
like such?

Fix for $record->{restrict_allow} which was being clobbered by $opt values.


abbreviated logic from how I read this.

my %o_default_defined = (
mv_update_empty => 1,
restrict_allow => 'page area var',
);

sub editor {
resolve_options($opt);
display({restrict_allow => $opt->{restrict_allow}})
}

sub resolve_options {
if ($opt->{cgi} && $CGI->{restrict_allow})
$opt->{restrict_allow} = $CGI->{restrict_allow}
}

# defaults only set if no value exists
while( my ($k, $v) = each %o_default) {
$opt->{$k} ||= $v;
}
}

sub display {
# $opt->{restrict_allow} is defined or default now.
# $record->{restrict_allow} from meta_editor

# no dice for $record
$opt->{restrict_allow} ||= $record->{restrict_allow};
}


Warm Regards,

Sam Batschelet
End Point Corporation

_______________________________________________
interchange-users mailing list
interchange-users@icdevgroup.org
http://www.icdevgroup.org/mailman/listinfo/interchange-users

Quoting Sam Batschelet (samb@endpoint.com):
>
> On 05/06/2016 05:23 PM, Mike Heins wrote:
> > Quoting Jon Jensen (jon@endpoint.com):
> >> On Fri, 6 May 2016, Mike Heins wrote:
> >>
> >> It merges the two sets of restrict_allow values together.
> >
> > Hmm. I don't understand the comment then, "was being clobbered by
> > default values". This more looks like a fix for the default values
> > getting clobbered by the $opt, which I would think is the intention
> > of the code.
> >
> > In other words, if you set it in $opt it should blow away what's in
> > $record, as presumably your setting of the option should indicate that
> > you know what's best. .
>
> Mike,
> My understanding is $opt->{restrict_allow} was defined by the
> o_default_defined hash or $CGI->{restrict_allow} values.

Maybe, and maybe not. If you didn't set anything in $opt->{restrict_allow},
sure. But if you wanted to disallow "area" or "var", you are now out
of luck.

> And as $opt
> was always defined, if you set restrict_allow via meta_editor,
> $record->{restrict_allow} would never be set by this line.
>
> $opt->{restrict_allow} ||= $record->{restrict_allow};
>
> So perhaps the word clobbered was dramatic, but you could not define
> $opt even if it were only the default values with $record. As Jon said
> the patch takes the $opt values and merges them with $record. This way
> you can maintain defaults as well as possible input options. Certainly
> open to better way to handle this. The commit message would read better
> like such?
>
> Fix for $record->{restrict_allow} which was being clobbered by $opt values.

I do understand, but the problem is this -- you can never override
$record->{restrict_allow} with $opt this way. And there is an easy way to
get what you want -- just add "area var" to any restrict_allow value you
set -- problem solved.

>
>
> abbreviated logic from how I read this.
>
> my %o_default_defined = (
> mv_update_empty => 1,
> restrict_allow => 'page area var',
> );
>
> sub editor {
> resolve_options($opt);
> display({restrict_allow => $opt->{restrict_allow}})
> }
>
> sub resolve_options {
> if ($opt->{cgi} && $CGI->{restrict_allow})
> $opt->{restrict_allow} = $CGI->{restrict_allow}
> }
>
> # defaults only set if no value exists
> while( my ($k, $v) = each %o_default) {
> $opt->{$k} ||= $v;
> }
> }
>
> sub display {
> # $opt->{restrict_allow} is defined or default now.
> # $record->{restrict_allow} from meta_editor
>
> # no dice for $record
> $opt->{restrict_allow} ||= $record->{restrict_allow};
> }

Bottom line is that I don't see the benefit of having these
shadow values preserved by this code -- if you are setting
restrict_allow, you might as well add " area var " to it.

When I think about it, there is some question as to whether
you might get unexplained behavior because you don't understand
that "page area var" are not intrinsically allowed but are instead
allowed via a default setting. But if you know enough to do the
setting, you should quickly be able to diagnose why your [var FOO] tag
doesn't get interpolated.

Remember that you can set $opt with the tag call, too -- it is
not always done via CGI. In the admin, especially.

--
Mike Heins
End Point -- Expert Internet Consulting http://www.endpoint.com/
phone +1.765.253.4194 <mikeh@endpoint.com>

Experience is what allows you to recognize a mistake the second time you
make it. -- unknown

_______________________________________________
interchange-users mailing list
interchange-users@icdevgroup.org
http://www.icdevgroup.org/mailman/listinfo/interchange-users

On 10/05/16 01:23, Mike Heins wrote:
> Bottom line is that I don't see the benefit of having these
> shadow values preserved by this code -- if you are setting
> restrict_allow, you might as well add " area var " to it.
>
> When I think about it, there is some question as to whether
> you might get unexplained behavior because you don't understand
> that "page area var" are not intrinsically allowed but are instead
> allowed via a default setting. But if you know enough to do the
> setting, you should quickly be able to diagnose why your [var FOO] tag
> doesn't get interpolated.
>
> Remember that you can set $opt with the tag call, too -- it is
> not always done via CGI. In the admin, especially.

Note also that someone may have explicitly wanted to override the
default values with the opt and now they can't due to this patch, so it
could very well break existing code.


Peter

_______________________________________________
interchange-users mailing list
interchange-users@icdevgroup.org
http://www.icdevgroup.org/mailman/listinfo/interchange-users