Make WordPress Core

Opened 3 years ago

Closed 15 months ago

Last modified 15 months ago

#54225 closed enhancement (fixed)

Request header key case inconsistencies

Reported by: johnjamesjacoby's profile johnjamesjacoby Owned by: audrasjb's profile audrasjb
Milestone: 6.2 Priority: normal
Severity: normal Version: 2.7
Component: HTTP API Keywords: has-patch has-unit-tests commit
Focuses: docs Cc:

Description

RFCs 7230 and 7540 define header keys as case-insensitive.

WordPress is consistently inconsistent when it comes to :

  • Retrieving headers – always lowercase:
    wp_remote_retrieve_header( $response, 'content-md5' )`
    
  • Setting headers – almost always Capital-Case:
    header( 'Content-Type: text/html; charset=utf-8' );
    
  • Sitemaps code (from 5.5) uses Title-case:
    header( 'Content-type: application/xml; charset=UTF-8' );
    

(Thankfully, when it comes to actually using the wp_remote_retrieve_header() function (and subsequently the Requests API and WP4.6) it resolves down to Requests_Utility_CaseInsensitiveDictionary so it is all working as defined in the specification.)

Possible code/docs improvements here include:

  • standardizing when core uses which case
  • explicitly stating that header key parameters are case-insensitive

Related to: #51736, #38231.

Attachments (1)

54225.patch (86.3 KB) - added by mhkuu 20 months ago.

Download all attachments as: .zip

Change History (21)

#1 @johnjamesjacoby
3 years ago

  • Focuses docs added

#2 @costdev
2 years ago

  • Type changed from defect (bug) to enhancement

#3 @hellofromTonya
2 years ago

  • Milestone changed from 5.9 to 6.0

5.9 is in feature freeze and Beta 1 release is < 4 hours away, moving this to 6.0.

#4 @costdev
2 years ago

  • Milestone changed from 6.0 to 6.1

With Beta 1 released yesterday, I'm moving this ticket to the 6.1 milestone.

#5 in reply to: ↑ description @SergeyBiryukov
2 years ago

Replying to johnjamesjacoby:

Possible code/docs improvements here include:

  • standardizing when core uses which case
  • explicitly stating that header key parameters are case-insensitive

Looking at the core files (excluding external libraries):

  • Content-Type
    • 89 instances of Content-Type
    • 27 instances of content-type
    • 9 instances of Content-type
  • Content-Disposition
    • 13 instances of Content-Disposition
    • 2 instances of content-disposition
  • Content-MD5
    • 1 instance of Content-MD5
    • 1 instance of content-md5

These numbers include DocBlocks and inline comments, so may not be 100% accurate, but should work as a quick overview. It looks like we can standardize on Content-Type, Content-Disposition, Content-Type, etc.

@mhkuu
20 months ago

#6 @mhkuu
20 months ago

I've created a patch:

  • Header creation now always used the capitalized forms, also with $request->add_header and $request->set_header (mainly called in the tests)
  • Calls to wp_remote_retrieve_header and wp_get_http_headers now always use a capitalized term as the second argument.
  • Docblocks and inline comments are updated to reflect the fact that we use capitalized terms in the code.
  • I also updated the deprecated files src/wp-includes/class-json.php and src/wp-includes/class-snoopy.php for consistency.
  • The return value of wp_get_http_headers was updated to reflect that a Requests_Utility_CaseInsensitiveDictionary is returned, rather than a string.

This ticket was mentioned in PR #3243 on WordPress/wordpress-develop by mhkuu.


20 months ago
#7

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

Improves the consistency in capitalization of fetching and outputting of request headers. See the Trac ticket for details.

Trac ticket: https://core.trac.wordpress.org/ticket/54225

#8 @SergeyBiryukov
20 months ago

In 54157:

Docs: Correct @return value for wp_get_http_headers().

Following the update to replace the HTTP API internals with Requests library in WordPress 4.6, the return value of wp_remote_retrieve_headers() has changed from a simple array to an object which implements ArrayAccess.

Since wp_get_http_headers() directly returns the result of wp_remote_retrieve_headers(), its return value should reflect that change.

Includes:

  • Updating the return value for the deprecated wp_get_http() function, which also directly returns the result of wp_remote_retrieve_headers().
  • Minor DocBlock formatting changes for some other HTTP API functions per the documentation standards.

Follow-up to [2416], [6390], [8092], [9013], [37428], [37989], [38730].

Props mhkuu.
See #54225, #55646.

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


19 months ago

#10 @JeffPaul
19 months ago

  • Milestone changed from 6.1 to 6.2

As discussed ahead of today's scheduled 6.1 Beta 1 release party (see the Slack link above), it was deemed that this ticket was not ready and should be punted to the 6.2 milestone to be reviewed and addressed then.

#11 @mhkuu
18 months ago

I've merged trunk into the PR and resolved the merge conflicts.

#12 @costdev
15 months ago

  • Keywords changes-requested added; 2nd-opinion removed

I've requested that two external library files are removed from PR 3243. Adding the changes-requested keyword.

Also looks like the 2nd-opinion keyword was added at the ticket's creation. As this ticket has commits, I think we're good to remove this keyword now.

#13 @mhkuu
15 months ago

  • Keywords changes-requested removed

Thanks @costdev for checking! I've removed the changes to these external library files. I also merged trunk into the PR and resolved the merge conflicts.

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


15 months ago

#15 @costdev
15 months ago

  • Keywords commit added

This ticket was discussed during the bug scrub. Feedback on the PR has been addressed and this looks ready for commit consideration. Adding the appropriate keyword.

Additional props: @audrasjb @petitphp

#16 @audrasjb
15 months ago

  • Owner set to audrasjb
  • Resolution set to fixed
  • Status changed from new to closed

In 55210:

HTTP API: Fix request header inconsistencies.

This changeset improves the consistency in capitalization of fetching and outputting of request headers. It also updates occurrences found in some docblocks.

Props johnjamesjacoby, costdev, audrasjb, petitphp, mhkuu, SergeyBiryukov.
Fixes #54225.

#18 @audrasjb
15 months ago

In 55211:

HTTP API: Fix a unit test failure found after [55210].

Follow-up to [55210].

See #54225.

#19 @SergeyBiryukov
15 months ago

In 55212:

HTTP API: Restore one instance of the X-Pingback header capitalization.

The revert in the previous commit appears to be accidental.

Follow-up to [55210], [55211].

See #54225.

#20 @SergeyBiryukov
15 months ago

In 55213:

Docs: Capitalize X-Pingback in discover_pingback_server_uri() DocBlock.

Follow-up to [55210], [55211], [55212].

See #54225.

Note: See TracTickets for help on using tickets.