Problem/Motivation
In order to prepare for resolving #2364011: [meta] External caches mix up response formats on URLs where content negotiation is in use we want to refactor our code so we only use $request->getRequestFormat()
for resolving the requested format. This will decouple the rest of our code from the actual implementation.
Proposed resolution
The first step is to use the request format internally.
This step does not solve yet the problem of #2364011: [meta] External caches mix up response formats on URLs where content negotiation is in use but is a first step towards it
Remaining tasks
User interface changes
API changes
Beta phase evaluation
Issue category | Task, because itself just prepare the code to solve #2364011: [meta] External caches mix up response formats on URLs where content negotiation is in use |
---|---|
Issue priority | Normal, even if its a subissue of a critical |
Disruption | Not disruptive, as you can still call out to the ContentNegotation. In general we though need a proper change record for the meta. |
Comment | File | Size | Author |
---|---|---|---|
#21 | interdiff.txt | 2.57 KB | dawehner |
#21 | 2445723-21.patch | 34.62 KB | dawehner |
#12 | 2445723-12.interdiff.txt | 4.34 KB | neclimdul |
#12 | 2445723-12.patch | 34.46 KB | neclimdul |
#7 | interdiff.txt | 6.3 KB | dawehner |
Comments
Comment #1
dawehnerComment #2
dawehnerHere is a first quick try.
Comment #4
dawehnerI hate assigned issues.
Comment #5
dawehnerTwo small bugfixes.
Comment #7
dawehnerThe exception actually uses the wrong content-type in HEAD, so we have to fix that.
Comment #8
Crell CreditAttribution: Crell commentedThere's a base class this can extend instead, no? We already build a nicely extensible exception handling setup. We can use that instead.
Comment #9
dawehnerWell, for the HAL_JSON subscriber we actually want to cover not just a limited set of HTTP exceptions (403/404) but way more,
422/406 and what not. Maybe we should provide a way for those exceptions to call out to a generic method for all other exceptions?
Comment #10
Crell CreditAttribution: Crell commentedhttp://cgit.drupalcode.org/drupal/tree/core/lib/Drupal/Core/EventSubscri...
It will handle any status code you make a method for, automagically. If you don't, a generic one should kick in already.
http://cgit.drupalcode.org/drupal/tree/core/lib/Drupal/Core/EventSubscri...
We got this. :-)
Comment #11
dawehnerRight, shoudl I apply 400...40N?
So rest throws:
Comment #12
neclimdulSpent some time with this today.
I think @dawehner is saying that the base class is a bit clunky if you want to respond to exception ranges or generalize exceptions. This is definitely true. I started to play with showing that which led me to look into what HAL exceptions are suppose to look like. Turns out there is no mention of it in the standard which made me sad. We didn't have an implementation, we didn't add testing, and we know we're going to get into exceptions more as part of #2364011 so I just took it out.
I also noticed dawehner cleverly moved the request format registering into the middleware. hal.module was converted to use this but it also had the old subscriber. The old subscriber seemed unnecessary so I took it out too.
Comment #13
Crell CreditAttribution: Crell commentedWe discussed using VndError or ApiProblem in the past, but neither one is a stable spec yet. If I get around to it I plan to add an ApiProblem contrib module.
Comment #14
neclimdulUpdated summary to make it clear that this patch is just a refactor to decouple most of our code so we can move forward more effectively. Can we move this to RTBC? I don't see that there are any concerns with this at the moment and its mostly just cleaning up code.
Comment #15
Crell CreditAttribution: Crell commentedYes please. Even if we remove the middleware later at least this makes getRequestFormat() useful, and that's a win.
Comment #18
neclimdulbot irregularity
Comment #19
Wim LeersThis should also bring a minor performance improvement, since less code needs to run on each request: no event subscribers to add these formats; it's done at container building time :)
Overall: YAY! But a few nits. I'd feel bad to NW this, so keeping at RTBC so it can be fixed in a minor follow-up. That allows us to continue faster in the parent issue.
\o/
\o/
negotiator
(because the property is named that way)
Incomplete docs.
s/mime/MIME/
?
\o/
"and then an ordinary html on"
-> cannot parse that :)
Comment #20
znerol CreditAttribution: znerol commentedPlease add a @todo here, the middleware is temporary and needs to be removed ASAP again.
Comment #21
dawehnerDone.
Comment #22
Fabianx CreditAttribution: Fabianx for Drupal Association commentedRTBC + 1
Comment #23
Wim LeersAnother quick reroll for #20?
Comment #24
neclimdulIts there
Comment #25
Wim LeersOh, oops :) I overlooked that. Excellent :)
Comment #26
Crell CreditAttribution: Crell commentedTagging.
Comment #27
alexpottCommitting this as part of the fix to #2364011: [meta] External caches mix up response formats on URLs where content negotiation is in use and the fact the it reduces complexity and has a performance benefit. Committed 13e0794 and pushed to 8.0.x. Thanks!
Comment #30
andypostThere's follow-up #2725435: Remove outdated @todo pointing to #2364011