Re: [interchange] Refactor robot testing into an internal routine

> On Jan 17, 2017, at 10:04 PM, Peter <[email protected]> wrote:
> 
> On 18/01/17 11:18, David Christensen wrote:
>> +	$Vend::Robot = check_is_robot();
>> +
>> +	$CGI::values{mv_tmp_session} ||= 1 if $Vend::Robot;
>> +}
>> +
>> +
>> +sub check_is_robot {
>> +    my $ret = 1;
>> +
>> #::logDebug("Check robot UA=$Global::RobotUA IP=$Global::RobotIP");
>> 	if ($Global::RobotIP and $CGI::remote_addr =~ $Global::RobotIP) {
>> #::logDebug("It is a robot by IP!");
>> -		$Vend::Robot = 1;
>> +		$ret = 1;
>> 	}
>> 	elsif ($Global::HostnameLookups && $Global::RobotHost) {
>> 		if (!$CGI::remote_host && $CGI::remote_addr) {
>> @@ -291,7 +300,7 @@ EOF
>> 		}
>> 		if ($CGI::remote_host && $CGI::remote_host =~ $Global::RobotHost) {
>> #::logDebug("It is a robot by host!");
>> -			$Vend::Robot = 1;
>> +			$ret = 1;
>> 		}
>> 	}
>> 	unless ($Vend::Robot) { 
>> @@ -300,11 +309,10 @@ EOF
>> 		}
>> 		elsif ($Global::RobotUA and $CGI::useragent =~ $Global::RobotUA) {
>> #::logDebug("It is a robot by UA!");
>> -			$Vend::Robot = 1;
>> +			$ret = 1;
>> 		}
>> 	}
>> -
>> -	$CGI::values{mv_tmp_session} ||= 1 if $Vend::Robot;
>> +	return $ret;
>> }
> 
> I see some issues with this:
> 
> 1.  check_is_robot() will always return 1, I think you meant to
> initialize $ret to 0, not 1 at the top.
> 
> 2.  The previous code would not explicitly set $Vend::Robot to 0, only
> to 1, so there is an incompatible case where $Vend::Robot is already set
> to 1 here but check_is_robot() would return 0.  You can fix this by
> changing:
> 
> $Vend::Robot = check_is_robot();
> 
> ...to...
> 
> $Vend::Robot ||= check_is_robot();

Thanks for the feedback; I do know it’s a little wonky currently, thanks to some locally failing unit tests (yay!).

I tested a naïve implementation of the above suggestions and didn’t get things working as expected.  I also noticed an additional place where I referenced $Vend::Robot *inside* this block, which was unintentional.  I am going to revisit with a clear head tomorrow.

The intention is for check_is_robot() to just examine the relevant UA, etc, only and return a boolean determination, so $Vend::Robot should be =, not ||=; the only other places that refer to $Vend::Robot check for the flag, not having anything to do with it; the ||= comes into play when considering whether to set the mv_tmp_session, which we do respect the previous setting of.

Thanks again,

David
--
David Christensen
End Point Corporation
[email protected]
785-727-1171




_______________________________________________
interchange-users mailing list
[email protected]
http://www.icdevgroup.org/mailman/listinfo/interchange-users