coverage: 82.648%. remained the same
when pulling b932ecf on marcwieland95:preg_replace-deprecation
into 8f56e14 on wp-graphql:develop.

@justlevine the send_header method is typed in the docblock for phpstan, but is not typed in the function signature, and given that it's a public function that can be used by plugins to send headers along with the response, we need to move forward assuming that null values are being passed.

I'll update the docblock to reflect as much and this solution will convert null values to an empty string to play nice.

@justlevine I updated the docblock to reflect the reality of how the function is already being used. b932ecf

given that it's a public function that can be used by plugins to send headers along with the response, we need to move forward assuming that null values are being passed.

@jasonbahl there's a difference between assuming and encouraging though.

8151e94 was already enough to defensively prevent people from passing null IRL.

Changing the doc-block, however encourages people to pass null.

Question is what's the better path we want to encourage: to pass a header with an empty value (?string) or to only send a header if it has a value (string).

I strongly lean towards the latter.

@justlevine do you think it would make sense to add a deprecation notice within the send_header method to discourage null inputs?

i.e. something like:

if ( null === $value ) {
    _deprecated_argument( __METHOD__, '@since next-version', 'Passing null values to send_header is deprecated. Please pass a string value.' );
}

This way the docblock can accurately represent how the function currently works (regardless of ideals) and actively discourage users from using it in a way we don't encourage?

Or we can leave the docblock string as-is (even though null is technically valid) and let the defensive fix do it's thing in the mean time.

@justlevine do you think it would make sense to add a deprecation notice within the send_header method to discourage null inputs?

Yes, I'd want to mark this as deprecated for signature change in a future release, but i'd rather wait until we know when that release might happen, and not prematurely force it for 3.0. There's no reason to cause a break over this, so we can slate it strategically to occur alongside related changes.

Or we can leave the docblock string as-is (even though null is technically valid) and let the defensive fix do it's thing in the mean time.

In lieu of a method signature, the docs define what is "technically valid" (regardless if PHP throws an error), so yeah even if there wasn't a good reason to temporarily hold off on event logging, I personally wouldn't feel too bad about the fact that passing null doesn't cause an issue despite the function wanting a string 🤷

Code Climate has analyzed commit 43dda0d and detected 0 issues on this pull request.

View more on Code Climate.

Looks like this PR got reverted in this commit: 835321e