Perhaps, your module should not fetch the data at all, but just accept the data as a scalar value. Both would also allow for easier testing.

That is exactly what I was thinking!

In the hope it clarifies Bod's question, I think the module in question is Business::Stripe::Webhook, whose version 1.0 constructor is:

sub new {
    my $class = shift;
    my %vars = @_;

    $vars{'error'}      = '';

    $vars{'reply'}      =  {
        'status'        => 'noaction',
        'sent_to'       => [ ],
        'sent_to_all'   => 'false',
    };
    
    if (exists $ENV{'GATEWAY_INTERFACE'}) {
        read(STDIN, $vars{'payload'}, $ENV{'CONTENT_LENGTH'});
        $vars{'webhook'} = decode_json($vars{'payload'}) if $vars{'pay
+load'};
        $vars{'error'}   = 'No payload data' unless $vars{'webhook'};
    } else {
        $vars{'error'}   = 'Looks like this is not a web request!';
    }
    
    return bless \%vars, $class;
}
[download]

Though I'm definitely not a Web programmer, from an interface and TDD point of view, I pulled a face the instant I saw the constructor using an environment variable to decide whether to read from STDIN or not.

It seems clearer and easier to test if this module were to simply accept a payload property. That way, the module's tests can easily pass in all sorts of dodgy payloads to see how it handles bad input.

That is, instead of trying to do everything in one module, use several, smaller, more cohesive modules to get the job done.

Though I'm definitely not a Web programmer, from an interface and TDD point of view, I pulled a face the instant I saw the constructor using an environment variable to decide whether to read from STDIN or not.

That's clearly CGI. Or, in other words, this module won't work with anything but CGI scripts. Maybe, but just maybe, the magic implemented by FCGI/CGI::Fast resp. ModPerl::Registry might be sufficient to fake enough CGI environment (i.e. faked environment, faked STDIN) to make it work.

Oh, and if you use some CGI library with that module, it is a question of initialisation order if this module will work or not. CGI prefers to read STDIN by itself, and I think it won't be happy if you steal its input data. If you init CGI first, input data for Bod's module is gone.

So, in any case, passing the JSON data as a parameter is way better.

Alexander

--
Today I will gladly share my knowledge and experience, for there are no sweeter words than "I told you so". ;-)

Oh, and if you use some CGI library with that module, it is a question of initialisation order if this module will work or not

Having hit this exact problem whilst creating the module, there is a warning about this in the docs.

sub new {
    my $class = shift;
    my %vars = @_;

    $vars{'error'}      = '';

    $vars{'reply'}      =  {
        'status'        => 'noaction',
        'sent_to'       => [ ],
        'sent_to_all'   => 'false',
    };
    
    if (!exists{'payload'}) {
        # Obtaining  payload from STDIN only 
        # exists for backward  compatability
        # This option is deprecated and will
        # be  removed  in  a  future version
        if (exists $ENV{'GATEWAY_INTERFACE'}) {
            read(STDIN, $vars{'payload'}, $ENV{'CONTENT_LENGTH'});
            $vars{'webhook'} = decode_json($vars{'payload'}) if $vars{
+'payload'};
            $vars{'error'}   = 'No payload data' unless $vars{'webhook
+'};
        } else {
            $vars{'error'}   = 'Looks like this is not a web request!'
+;
        }
    }
    
    return bless \%vars, $class;
}
[download]

It seems clearer and easier to test if this module were to simply accept a payload property

Is that a better constructor?
The production version of the module has been released so I don't want to break that. So the new method now accepts a payload property. If that is missing and it is running on a webserver with the right environment, it falls back to the previous behaviour.

A couple of versions down the line I will remove the fallback code and set an error condition if payload is not a valid JSON scalar.

Is that a better constructor?

Looks like a much better interface to me ... erm, after you fix the syntax error, by changing:

if (!exists{'payload'}) {
[download]

to:

if (!exists $vars{'payload'}) {
[download]

;-) ... with the caveat that I have zero experience in the CGI domain.

You should also add some new tests with dodgy payloads to verify they're being rejected with a useful error message.