#2934152: ContentTypeHeaderMatcher should not run for GET, HEAD, OPTIONS or TRACE requests improved ContentTypeHeaderMatcher's logic. It's a correct bugfix. But over at #2934149-19: [>=8.5] JSON API routes not specifying _content_type_format route requirement, resulting in bad DX we just noticed that there is another bug/oversight: DELETE requests. DELETE requests never have a request body. And so it doesn't make sense to require a Content-Type request header either.

CommentFileSizeAuthor
#6 2971012-6.patch1.68 KBwim leers
#6 interdiff.txt1.13 KBwim leers
#3 2971012-3.patch1.56 KBwim leers
#3 interdiff.txt1011 byteswim leers
#2 2971012-2.patch604 byteswim leers

Comments

Wim Leers created an issue. See original summary.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Active » Needs review
StatusFileSize
new 2971012-2.patch604 bytes

Failing test.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Assigned: wim leers » Unassigned
StatusFileSize
new interdiff.txt1011 bytes
new 2971012-3.patch1.56 KB

And the fix.

The last submitted patch, 2: 2971012-2.patch, failed testing. View results

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented

This has test-coverage and looks to be really simple. I'm not to sure about the documentation. I think it'd be better do do something like this:

    // The Content-type header does not make sense on GET or DELETE requests, 
    // they do not carry any content. Nothing to filter in this case. Same
    // for all other safe methods.

However, It's just a suggestion.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
StatusFileSize
new interdiff.txt1.13 KB
new 2971012-6.patch1.68 KB

#5: I knew that comment was off! Thanks 🙏

Fixed!

P.S.: thank you very much for the review! How did you find this issue so fast!? 😲

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Needs review » Reviewed & tested by the community

That looks better!

I had some time and was looking at the needs review queue to see if I could help with any of the open issues.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

I had some time and was looking at the needs review queue to see if I could help with any of the open issues.

<3

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed 246a31c42f to 8.6.x and e3004ccad6 to 8.5.x. Thanks!

  • alexpott committed 246a31c on 8.6.x 
    Issue #2971012 by Wim Leers, borisson_: ContentTypeHeaderMatcher should...

  • alexpott committed e3004cc on 8.5.x 
    Issue #2971012 by Wim Leers, borisson_: ContentTypeHeaderMatcher should...
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

:O

This was one of my fastest core commits EVER. Thank you so much @alexpott!

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

And bam, #2934149-25: [>=8.5] JSON API routes not specifying _content_type_format route requirement, resulting in bad DX is now passing the tests that previously were failing! ❤️

Status: Fixed » Closed (fixed)

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