Pass all http headers in request metadata

[steve-nester-uk]

PLAT-4629

Goal

The notifier currently only append headers that start with HTTP_ prefix. Instead all headers should be sent.

Changeset

Update the BasicResolver to use getallheaders if it exists, this will set all HTTP headers

Tests

Update to existing RequestTest.php

Discussion

Alternative Approaches

Outstanding Questions

Linked issues

Review

For the submitter, initial self-review:

• [ x] Commented on code changes inline explain the reasoning behind the approach
• [ x] Reviewed the test cases added for completeness and possible points for discussion
• A changelog entry was added for the goal of this pull request
• [ x] Check the scope of the changeset - is everything in the diff required for the pull request?
• This pull request is ready for:
  • [ x] Initial review of the intended approach, not yet feature complete
  • [ x] Structural review of the classes, functions, and properties modified
  • [ x] Final review

For the pull request reviewer(s), this changeset has been reviewed for:

• Consistency across platforms for structures or concepts added or modified
• Consistency between the changeset and the goal stated above
• Internal consistency with the rest of the library - is there any overlap between existing interfaces and any which have been added?
• Usage friction - is the proposed change in usage cumbersome or complicated?
• Performance and complexity - are there any cases of unexpected O(n^3) when iterating, recursing, flat mapping, etc?
• Concurrency concerns - if components are accessed asynchronously, what issues will arise
• Thoroughness of added tests and any missing edge cases
• Idiomatic use of the language

[imjoehaines]

Is Travis running with or without getallheaders being called? I assume without because it's not running a server

It'd be nice to add tests that prove we call it when it exists, or maybe we should leave it out to avoid over complicating things, because we know the current output is fine (once we add the missing headers)?

[imjoehaines]

LGTM!

Can you add a changelog entry too? Just use TBD as the version name for now 🙂

[GrahamCampbell]

We have to be careful that we're not sending sensitive data now, by default. We should redact the Authorization header, and perhaps other custom headers, maybe those with keys that contain "auth" or "key".

[imjoehaines]

We have to be careful that we're not sending sensitive data now, by default. We should redact the Authorization header, and perhaps other custom headers, maybe those with keys that contain "auth" or "key".

What extra headers are included by getallheaders that aren't in $_SERVER as HTTP_x? Authorization is $_SERVER['HTTP_AUTHORIZATION'], for example, so it's already being sent by default

[GrahamCampbell]

Oh, maybe that behaviour should be revisited. ;)

[imjoehaines]

Oh, maybe that behaviour should be revisited. ;)

Yeah, we filter those headers on other platforms so should bring PHP in line for sure 🙂