Re^3: STDIN typeglob

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.

Comment on Re^3: STDIN typeglob Select or Download Code

Replies are listed 'Best First'.
Re^4: STDIN typeglob by afoken (Chancellor) on Jun 12, 2023 at 19:53 UTC
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". ;-)	[reply]
Re^5: STDIN typeglob by Bod (Parson) on Jun 13, 2023 at 20:42 UTC
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.	[reply]
Re^4: STDIN typeglob by Bod (Parson) on Jun 13, 2023 at 20:58 UTC
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.	[reply] [d/l] [select]
Re^5: STDIN typeglob by eyepopslikeamosquito (Archbishop) on Jun 14, 2023 at 00:24 UTC
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.	[reply] [d/l] [select]
Re^6: STDIN typeglob by Bod (Parson) on Jun 18, 2023 at 17:13 UTC
Looks like a much better interface to me... Well, it's now on its journey from PAUSE to CPAN...this and a few other enhancements have been included in Business::Stripe::Webhook `v1.10`	[reply] [d/l]