[ic] [interchange] Add better messages, support for application/json handling

Mark Johnson mark at endpoint.com
Wed Dec 31 04:50:52 UTC 2014


On 12/30/2014 07:29 PM, Peter wrote:
> On 12/31/2014 11:50 AM, David Christensen wrote:
>>  use strict;
>>  
>>  {
>> -    local $@;
>> -    eval {
>> -        require JSON;
>> -    };
>> -    unless ($@) {
>> -        $Has_JSON = 1;
>> -    }
>> +	local $@;
>> +	eval {
>> +		require JSON;
>> +	};
>> +	if ($@) {
>> +		::logGlobal(
>> +			$@ =~ /^Can't locate JSON/ ?
>> +			'No POST support for application/json without installing JSON module' :
>> +			"Error loading JSON module: $@"
>> +		);
>> +	}
>> +	else {
>> +		$Has_JSON = 1;
>> +	}
>>  }
> 
> One correction and a few suggestions...
> 
> $Has_JSON will puke because it's undeclared and we use strict.  You want:
> our $Has_JSON = 1;

$Has_JSON is not undeclared. It is defined in the use vars declaration.

> You don't have to test for $@ if you put the our $Has_JSON = 1 in the
> eval block like so:
> 
> our $Has_JSON;
> eval {
>     require JSON;
>     $Has_JSON = 1;
> };
> 
> if (!$Has_JSON) {
>     # ... error here (see below)?
> }

6 of one, 1/2 dozen of the other.

> The other thing is that if someone doesn't have JSON it's not really an
> error and shouldn't be reported as such, unless they try to enable
> UnpackJSON.  I suggest that UnpackJSON should be *off* by default (to
> maintain the status quo) and if it's attempted to be turned on with the
> JSON module present *then* you record an error (and even crash the
> catalog loading at that point).

It isn't reported as an error in the situation that the module just
isn't present. It's advisory, like quite a few other notices that are
presented at startup. However, we did debate whether to enable that
message or not. Our thoughts were that it would help to advertise the
feature once available and, if it were too much to bear, one could start
with --quiet.

UnpackJSON is not necessary for handling posts of type application/json.
One could just as easily grab the JSON structure out of $CGI::json_ref
in global code and go to town. So that alone as an indicator isn't
sufficient to determine whether its use is desired.

I have no strong opinion on whether UnpackJSON should be on or off by
default, but neither approach is status quo since the entire handler is new.

Mark



More information about the interchange-users mailing list