Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

uninitialized warning issued when stringifying a request #2232

Open
karenetheridge opened this issue Feb 20, 2025 · 6 comments
Open

uninitialized warning issued when stringifying a request #2232

karenetheridge opened this issue Feb 20, 2025 · 6 comments

Comments

@karenetheridge
Copy link
Contributor

What am I doing wrong here? This seems like a warning that shouldn't happen...

use strict;
use warnings;
use Mojo::Message::Request;
use Data::Dumper;

my $req = Mojo::Message::Request->new->error({ message => 'error' });
print STDERR "### req is ", Dumper($req);

my $str = $req->to_string;
print STDERR "### got string\n$str\n";

produces:

### req is $VAR1 = bless( {
                 'error' => {
                              'message' => 'error'
                            },
                 'finished' => 1,
                 'events' => {},
                 'state' => 'finished'
               }, 'Mojo::Message::Request' );
Use of uninitialized value $_ in concatenation (.) or string at /Volumes/chambord/Users/ether/.perlbrew/libs/41.7@std/lib/perl5/Mojo/Headers.pm line 181.
### got string
GET / HTTP/1.1
Host: 

This is with Mojolicious 9.39.

@karenetheridge
Copy link
Contributor Author

karenetheridge commented Feb 20, 2025

The undef warning is coming from the host header being undef. I can't find where in the code this header is being set; the first place I've identified where it is set is here:

### in Mojo::Content get_header_chunk, self is $VAR1 = bless( {
                 'asset' => bless( {
                                     'auto_upgrade' => 1
                                   }, 'Mojo::Asset::Memory' ),
                 'body_size' => 0,
                 'events' => {
                               'read' => [
                                           sub { "DUMMY" }
                                         ]
                             },
                 'headers' => bless( {
                                       'headers' => {
                                                      'host' => [
                                                                  undef
                                                                ]
                                                    }
                                     }, 'Mojo::Headers' ),
                 'read' => $VAR1->{'events'}{'read'}[0]
               }, 'Mojo::Content::Single' );

..but earlier, in Mojo::Message::fix_headers, host is not set.

@s1037989
Copy link
Contributor

According to spec, a host header must be sent with all http 1.1 requests.. I reckon Mojo is following spec somewhere -- like you, I'm curious to learn where!

@guest20
Copy link

guest20 commented Feb 20, 2025

The pod shows two ways to construct Mojo::Requests and they're both methods named "parse"... One that takes a url and one that takes http.

A request without a url sounds like a zen kōan

@karenetheridge
Copy link
Contributor Author

If I do $req->headers->host(''); there is still a warning, but then this change is enough to fix it:

diff --git a/lib/Mojo/Message/Request.pm b/lib/Mojo/Message/Request.pm
index c2b12398d..99b272707 100644
--- a/lib/Mojo/Message/Request.pm
+++ b/lib/Mojo/Message/Request.pm
@@ -73,7 +73,7 @@ sub fix_headers {
 
   # Host
   my $url = $self->url;
-  $headers->host($url->host_port) unless $headers->host;
+  $headers->host($url->host_port) unless defined $headers->host;
 
   # Basic authentication
   if ((my $info = $url->userinfo) && !$headers->authorization) {

@karenetheridge
Copy link
Contributor Author

karenetheridge commented Feb 22, 2025

By my reading of https://www.rfc-editor.org/rfc/rfc9112.html#section-3.2-5, a Host header is mandatory, but an empty value is permissible. Therefore we should be doing a definedness check (as in the patch above), not a truthy check. (0 is also a weird, but valid, Host value, which is also currently excluded.)

fix_headers() could also set the Host header to '' if it is not already set.

@kraih
Copy link
Member

kraih commented Feb 22, 2025

Seems reasonable.

karenetheridge added a commit to karenetheridge/Mojolicious that referenced this issue Feb 24, 2025
RFC9112 §3.2-5 says that Host is mandatory, but can be an empty value.
resolves issue mojolicious#2232.
karenetheridge added a commit to karenetheridge/Mojolicious that referenced this issue Feb 25, 2025
RFC9112 §3.2-5 says that Host is mandatory, but can be an empty value.
resolves issue mojolicious#2232.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants