Follow-up to #2364011-173: [meta] External caches mix up response formats on URLs where content negotiation is in use
Parent issue closed but there are 2 todos
git grep 2364011
core/lib/Drupal/Core/EventSubscriber/AcceptNegotiation406.php:13: * @todo fix or replace this in https://www.drupal.org/node/2364011
core/lib/Drupal/Core/StackMiddleware/NegotiationMiddleware.php:11: * @todo This is a temporary solution, remove this in https://www.drupal.org/node/2364011
| Comment | File | Size | Author |
|---|---|---|---|
| #27 | after_patch.png | 46.61 KB | larisse |
| #27 | before_patch.png | 33.15 KB | larisse |
| #25 | 2725435-applied_patch.png | 37.37 KB | abhijith s |
| #23 | 2725435-23.patch | 582 bytes | guilhermevp |
| #4 | 2725435-3.patch | 582 bytes | naveenvalecha |
Comments
Comment #2
andypostadded issues where that was introduced
btw 403/404 returns html render for REST endpoints and this is a bug
Comment #4
naveenvalechaHere's the intial patch. Assuming that the keeping the NegotiationMiddleware have a use case. So only removing the @todo atm. Not removing it due to B/C
//Naveen
Comment #5
20th commented@naveenvalecha
You should remove both occurrences of the
@todocomment because the referenced issue is closed for good.@andypost
I wish you provided more details on when and how REST points return HTML. I did some quick manual testing and the format of the 404 pages was what I expected it to be. But that bug might have been already patched in the 8 months that passed since your comment.
Comment #6
naveenvalechaComment #8
xjmLooks like this is covered in the scope of #2835626: Rename & document Drupal\Core\EventSubscriber\AcceptNegotiation406 KernelEvents::VIEW event subscriber.
Comment #9
borisson_The other issue only fixed this in
RenderArrayNonHtmlSubscriber, not in theNegotiationMiddlewarewhere the patch in #4 removes it from.Should we reopen this issue and commit #4 or do we open a new issue to resolve this?
Comment #10
andypostYes! patch from #4 exactly for that purpose
Comment #11
borisson_The patch in #4 still applies, so let's use that.
Comment #12
xjmCan we update or replace the @todo with whatever the current followup is (as described in #2)? We can't remove "temporary solutions" but we can deprecate them, so if we still think it's not-the-right-solution then we can file an issue for that.
The other alternative is "No actually, it's fine" in which case we can just remove it but tagging for subsystem maintainer feedback on whether the current API is actually what we want without any followup. :)
Comment #15
effulgentsia commentedSemi-related: #3028501: Enable header-based proactive content negotiation with optimizations and opt-outs available..
Comment #17
bradjones1Tagging related to json:api as this service is actually used in a non-deprecated, very important sense in
settingdeclaring theapi_jsonformat on requests. The properAcceptheader for json:api is part of the upstream spec so I don't think this can go anywhere now, in that light? Since it's required by a core module?Edited for a little clarity; the further I read it actually looks like the Accept header is part of the spec but not actively used in the negotiation; the request format is actually set by virtue of the route being accessed. Pretty clever. Regardless, though, it seems like this middleware probably can't go anywhere as the request formats have to be registered? So I think the todo would be good to go...
Comment #18
bradjones1This would get resolved in #3028501: Enable header-based proactive content negotiation with optimizations and opt-outs available. if that gets committed.
Comment #22
richardbporter commentedComment #23
guilhermevp commentedThere is only one of the todos remaining in 9.2.x to be removed in:
core/lib/Drupal/Core/StackMiddleware/NegotiationMiddleware.php:11: * @todo This is a temporary solution, remove this in https://www.drupal.org/node/2364011Comment #24
guilhermevp commentedComment #25
abhijith s commentedApplied patch #24 and it removes the
@todofrom the file.Comment #27
larisse commentedApplied patch #23 and it removes the @todo from the file for me too.
Comment #28
guilhermevp commentedIt's also applicable for 9.3.x.
Comment #29
alexpott@Abhijith S @larisse thank you for looking into this issue.
Posting screenshots of your codebase or CLI does not advance the issue, since the automated testing infrastructure tells us whether the patch applies correctly.
So, I've removed the issue credit for that screenshot. In the future, you can get credit for issues by reading the issue to understand its purpose, and posting your review or testing of that purpose. Thank you!
See the issue credit guidelines for more information.
Crediting @bradjones1 and @andypost for comments on the issue.
Comment #30
alexpottLooking at #3028501: Enable header-based proactive content negotiation with optimizations and opt-outs available. - that's extending the functionality of this middleware - so it's not going anywhere and this @todo is definitely outdated.
@guilhermevp there was no need to a new patch - somewhat unbelievably #4 still applied. I'm not going to remove issue credit this time because of the length of time between patches but if a patch does apply then doing a review rather than a new patch upload is way better. Please don't upload exactly the same patch as a previous person.
Committed d523586 and pushed to 9.3.x. Thanks!