Problem/Motivation

Tests that rely on the behavior of ContentTypeHeaderMatcher's content type check can fail in some nginx configurations. I'm marking this as minor because I don't think we really "support" running tests on nginx but at the same time this test failure exposes some inconsistent behaviors for rest resources when running on nginx where the improved DX from #2811133: Confusing error response set by ContentTypeHeaderMatcher when forgetting the Content-Type request header doesn't work as expected.

The problem comes from the fact that when using fast cgi configurations based on nginx example you will end up alwayspassing a content type to php regardless of whether the client supplied it. This causes the has check to fail say "yeah there's a header!" even though there really isn't.

nginx fastcgi reference: https://www.nginx.com/resources/wiki/start/topics/examples/full/#fastcgi...

Proposed resolution

Convert the check from has to a truthy test on the value. "" should is functionally the same as not existing so this should be fine.

Remaining tasks

- Patch
- Review

User interface changes

n/a

API changes

n/a

Data model changes

n/a

Release notes snippet

CommentFileSizeAuthor
#2 3090626-1.patch792 bytesneclimdul
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

neclimdul created an issue. See original summary.

neclimdul’s picture

Status: Active » Needs review
FileSize
792 bytes

Lets see if this breaks anything.

Wim Leers’s picture

Component: rest.module » routing system
Status: Needs review » Reviewed & tested by the community
Issue tags: +DX (Developer Experience), +API-First Initiative
Related issues: +#2811133: Confusing error response set by ContentTypeHeaderMatcher when forgetting the Content-Type request header

Makes sense, thank you!

alexpott’s picture

Version: 8.9.x-dev » 8.8.x-dev
Status: Reviewed & tested by the community » Fixed

This is the only instance of headers->has('Content-Type') in core.

Committed and pushed f995409be4 to 9.0.x and cb944b89bb to 8.9.x and 7dee9da351 to 8.8.x. Thanks!

Backported to 8.8.x as a test-only change.

  • alexpott committed f995409 on 9.0.x
    Issue #3090626 by neclimdul: ContentTypeHeaderMatcher dx tests broken...

  • alexpott committed cb944b8 on 8.9.x
    Issue #3090626 by neclimdul: ContentTypeHeaderMatcher dx tests broken...

  • alexpott committed 7dee9da on 8.8.x
    Issue #3090626 by neclimdul: ContentTypeHeaderMatcher dx tests broken...
mr.baileys’s picture

Wim Leers’s picture

#2860913: Use "missing content-type" exception message for empty content-type header. was not just a related issue, it was a duplicate issue that even predates this one.

Crediting people who worked on the duplicate issue.

neclimdul’s picture

Yeah, sorry. I searched but didn't get the right keywords I guess and failed to find it :(

Wim Leers’s picture

No worries, @neclimdul :)

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.