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

Update several status codes to RFC 9110 #197

Merged
merged 1 commit into from
May 27, 2024

Conversation

waterkip
Copy link
Contributor

@waterkip waterkip commented Apr 9, 2024

  • 306 is officially reserved but unused
  • 413 is called 'Content Too Large' in RFC 9110
  • 418 is officially reserved, so let's make it official here too
  • 422 is imported from WebDAV and named 'Unprocessable Content'.

Reference: https://www.rfc-editor.org/rfc/rfc9110.html#name-changes-from-rfc-7231

@oalders
Copy link
Member

oalders commented Apr 9, 2024

I'm going to close and re-open to see if that starts CI.

@oalders oalders closed this Apr 9, 2024
@oalders oalders reopened this Apr 9, 2024
Copy link
Member

@vanHoesel vanHoesel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need a proper way to fix HTTP_UNUSED

414 => 'URI Too Long',
415 => 'Unsupported Media Type',
416 => 'Range Not Satisfiable', # RFC 7233: Range Requests
417 => 'Expectation Failed',
# 418 .. 420
418 => "I'm a teapot", # RFC 9110: Its official now -sorta
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the registry indicates that it is (Unused)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. However becauee of the HTTP_UNUSED being there twixe I opted for keeping the Teapot reference. Also because the RFC refers to this fact as the reason why it assigned.

Copy link
Member

@vanHoesel vanHoesel Apr 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As much as it hurts my personal feeling that I'm a Teapot will be gone, I prefer correctness above sentiment, and let's stick with (Unused) :-(

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get what you are saying, but I do think this easter egg or April's day can stay. But you're in charge, so I'll remove it.

I've done some middle grounding based on another comment of yours in this PR.

In the compat section, when you call I_AM_A_TEAPOT, we set the status code of 418 to I'm a teapot and from that moment onwards you have the status message as I'm a teapot. It keeps the easter egg alive and kicking for those who get a little giggle out of it (myself included) and when you don't use it it stays (Unused). And we are skipping HTTP_UNUSED from being exported/implemented.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if we had Easter on 1-April in that year to0 ... butt yes, this is what I prefer, for those that ever used it to keep it alive.

lib/HTTP/Status.pm Show resolved Hide resolved
@vanHoesel
Copy link
Member

What about the following:

set the %StatusCodes like:

    306 => '(Unused)',                        # RFC 9110: Previously used and reserved
    418 => '(Unused)',                        # RFC 9110: Its official now -sorta
#   419 .. 420
my %compat = (
    I_AM_A_TEAPOT                 => sub {
                                       carp "HTTP Status Code 418 set to \"I'm a Teapot\"\n"
                                       $StatusCode{418} = "I'm a teapot";
                                       418
                                     },
    NO_CODE                       => \&HTTP_TOO_EARLY,
    PAYLOAD_TOO_LARGE             => \&HTTP_CONTENT_TOO_LARGE,
    REQUEST_ENTITY_TOO_LARGE      => \&HTTP_CONTENT_TOO_LARGE,
    REQUEST_RANGE_NOT_SATISFIABLE => \&HTTP_RANGE_NOT_SATISFIABLE,
    REQUEST_URI_TOO_LARGE         => \&HTTP_URI_TOO_LONG,
    UNPROCESSABLE_ENTITY          => \&HTTP_UNPROCESSABLE_CONTENT,
    UNORDERED_COLLECTION          => \&HTTP_TOO_EARLY,
    UNUSED                        => sub {
                                       carp "HTTP Status Code '(Unused)'\n";
                                       000
                                     },
);

That way, for those who fancy the 🫖 , it is still there when once used as HTTP_I_AM_A_TEAPOT. Maybe a nice way to only warn the user once for the teapot, and only set the status code message once ?

Please be advised, that use Carp might be needed, and that whilst looping during '# create mnemonic subroutines
' it will redefine HTTP_UNUSED.

Just some ideas...

@haarg
Copy link
Member

haarg commented Apr 10, 2024

I think it would be better to remove the constants for the "unused" codes. That eliminates the ambiguity between 306 and 418.

@vanHoesel
Copy link
Member

vanHoesel commented Apr 10, 2024

I think it would be better to remove the constants for the "unused" codes. That eliminates the ambiguity between 306 and 418.

I am totally fine with removing (or preferably not adding) the HTTP_UNUSED and RC_UNUSED.

But unlike non registered codes that will return undef when calling status_message for not registered codes, II think we should return (Unused) for both 306 and 418, for that is what is set to at the IANA registry.

@waterkip waterkip force-pushed the rfc9110 branch 2 times, most recently from f8f000d to 1c94d56 Compare April 10, 2024 12:53
@waterkip waterkip requested a review from vanHoesel April 10, 2024 12:56
Copy link
Member

@vanHoesel vanHoesel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@@ -121,7 +118,13 @@ die if $@;
push(@EXPORT, "RC_MOVED_TEMPORARILY");

my %compat = (
REQUEST_ENTITY_TOO_LARGE => \&HTTP_PAYLOAD_TOO_LARGE,
I_AM_A_TEAPOT => sub {
$StatusCode{418} = "I'm a teapot";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a really terrible idea. Changing global behavior based on a constant access should never happen. This should just return 418 and do nothing else.

The prototype also needs to be ().

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, about the prototype

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is a terrible idea, but so is it to just change the registry.

I would like to avoid the surprise:
say status_message HTTP_I_AM_A_TEAPOT; # (Unused)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If that is a large concern, just leave the message as "I'm a teapot".

The likelihood of anyone actually relying on 418 is very small.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use it in Production, and I often use it when developers can't get an agreement on the exact 4xx status

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And you are relying on what the message is?

There's just no way you can have this both ways. The message should either be (Unused) or I'm a teapot. I don't think it's particularly important which one. You can't do both. Trying to do both will make the result unpredictable and anything that relies on it will always be broken in some cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I favor the teapot, but if you want it to be unused, I'll go with that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we need to decide on one and the status code is unused anyway, I'm in favour of 🫖.

* 306 is officially reserved but unused
* 413 is called 'Content Too Large' in RFC 9110
* 418 is officially reserved, so let's make it official here too
* 422 is imported from WebDAV and named 'Unprocessable Content'.

Reference: https://www.rfc-editor.org/rfc/rfc9110.html#name-changes-from-rfc-7231

Signed-off-by: Wesley Schwengle <waterkip@cpan.org>
@waterkip
Copy link
Contributor Author

Any update on this PR?

@oalders
Copy link
Member

oalders commented May 26, 2024

Close and re-open to kick off CI.

@oalders oalders closed this May 26, 2024
@oalders oalders reopened this May 26, 2024
@oalders oalders merged commit 906d85f into libwww-perl:master May 27, 2024
41 checks passed
@oalders
Copy link
Member

oalders commented May 27, 2024

Thanks, @waterkip!

@waterkip waterkip deleted the rfc9110 branch May 27, 2024 20:17
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

Successfully merging this pull request may close these issues.

4 participants