WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#35426 closed defect (bug) (fixed)

Create response status code aliases on WP_Http for convenience

Reported by: joehoyle Owned by: joehoyle
Milestone: 4.5 Priority: normal
Severity: normal Version:
Component: HTTP API Keywords: has-unit-tests has-patch 2nd-opinion
Focuses: Cc:

Description

Throughout the REST API for response codes, we want to use aliased constants to be more clear in the code for each response code.

The best place to put these constants I think is WP_Http. This means in the REST API (and any other code) rather than specifying response codes directly (which is terse and opaque) one can use WP_Http::BAD_REQUEST for example.

Attachments (5)

35426.diff (2.8 KB) - added by joehoyle 4 years ago.
35426.2.diff (2.9 KB) - added by joehoyle 4 years ago.
35426.3.diff (3.0 KB) - added by joehoyle 4 years ago.
Added missing status codes and docblock
35426.tests.diff (676 bytes) - added by johnbillion 4 years ago.
35426.4.diff (568 bytes) - added by johnbillion 4 years ago.

Download all attachments as: .zip

Change History (16)

@joehoyle
4 years ago

@joehoyle
4 years ago

#2 @joehoyle
4 years ago

  • Owner set to joehoyle
  • Status changed from new to assigned

@joehoyle
4 years ago

Added missing status codes and docblock

#3 @joehoyle
4 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 36294:

Add response status code aliases on WP_Http for convenience.

These provide a more descriptive way to set response codes elsewhere,
so it's readable and less chance for the wrong response code to be
used such as 401 vs 403.

Props rmccue for the idea.
Fixes #35426.

#4 @johnbillion
4 years ago

  • Keywords needs-patch needs-unit-tests added
  • Resolution fixed deleted
  • Status changed from closed to reopened
  • Version trunk deleted

The 1xx statuses are missing.

#5 @johnbillion
4 years ago

  • Keywords has-unit-tests added; needs-unit-tests removed

@johnbillion
4 years ago

#6 @johnbillion
4 years ago

  • Keywords has-patch 2nd-opinion added; needs-patch removed

I wonder if these constant names need to be prefixed. CONTINUE is a reserved word in PHP so can't be used as a constant name.

Better: WP_Http::STATUS_CONTINUE, WP_Http::STATUS_OK, etc.

#7 @rmccue
4 years ago

@johnbillion That was actually intentional originally (I asked @joehoyle to leave them out), as it's almost impossible for those to get all the way up the stack. Additionally, you should probably never actually send them. They're essentially transactional status codes that are only used to negotiate the connection. 100 Continue i.e. indicates the server is ready to receive the rest of the message, 101 Switching Protocols is used to upgrade protocols (e.g. to HTTP/2). Most of these won't be handled correctly if you send them from WP, and they should never escape the internal handling of WP_HTTP.

That said, we might want them for coverage. If we have conflicts with keywords, I'd prefer we special-case those specific ones rather than all of them.

Last edited 4 years ago by rmccue (previous) (diff)

#8 @knutsp
4 years ago

When introduced, they should be used globally, right?

Will it be considered "refactoring just because we can" if these constants was implemented in get_status_header_desc()?

If not, I can create ticket and patch.

#9 @joehoyle
4 years ago

@knutsp give that the descriptions already exist in get_status_header_desc i'm not sure how much value there is to doing that, but I don't feel strongly either way.

@johnbillion I think we'd be better not renaming all constants with STATUS_* just for CONTINUE, I'd rather we give 100 a description that isn't a reserved word.

#10 @johnbillion
4 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

Fixed in r36749

This ticket was mentioned in Slack in #core by rmccue. View the logs.


4 years ago

Note: See TracTickets for help on using tickets.