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

Reference: https://www.drupal.org/core/beta-changes
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.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

Issue summary: View changes
dawehner’s picture

Status: Active » Needs review
FileSize
36.52 KB

Here is a first quick try.

Status: Needs review » Needs work

The last submitted patch, 2: 2445723-2.patch, failed testing.

dawehner’s picture

Assigned: dawehner » Unassigned

I hate assigned issues.

dawehner’s picture

Status: Needs work » Needs review
FileSize
38.82 KB
3.2 KB

Two small bugfixes.

Status: Needs review » Needs work

The last submitted patch, 5: 2445723-5.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
35.85 KB
6.3 KB

The exception actually uses the wrong content-type in HEAD, so we have to fix that.

Crell’s picture

+++ b/core/modules/hal/src/EventSubscriber/HalJsonExceptionSubscriber.php
@@ -0,0 +1,73 @@
+/**
+ * Provides exception pages for hal_json requests.
+ */
+class HalJsonExceptionSubscriber implements EventSubscriberInterface {

There's a base class this can extend instead, no? We already build a nicely extensible exception handling setup. We can use that instead.

dawehner’s picture

There's a base class this can extend instead, no? We already build a nicely extensible exception handling setup. We can use that instead.

Well, 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?

Crell’s picture

http://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. :-)

dawehner’s picture

It will handle any status code you make a method for, automagically. If you don't, a generic one should kick in already.

Right, shoudl I apply 400...40N?

So rest throws:

  • 415
  • 403
  • 404/li>
  • 400
  • 422
neclimdul’s picture

FileSize
34.46 KB
4.34 KB

Spent 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.

Crell’s picture

We 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.

neclimdul’s picture

Issue summary: View changes

Updated 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.

Crell’s picture

Status: Needs review » Reviewed & tested by the community

Yes please. Even if we remove the middleware later at least this makes getRequestFormat() useful, and that's a win.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 12: 2445723-12.patch, failed testing.

neclimdul queued 12: 2445723-12.patch for re-testing.

neclimdul’s picture

Status: Needs work » Reviewed & tested by the community

bot irregularity

Wim Leers’s picture

Issue tags: +Performance

This 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.

  1. +++ b/core/core.services.yml
    @@ -995,10 +1003,6 @@ services:
    diff --git a/core/lib/Drupal/Core/Ajax/AjaxSubscriber.php b/core/lib/Drupal/Core/Ajax/AjaxSubscriber.php
    
    diff --git a/core/lib/Drupal/Core/Ajax/AjaxSubscriber.php b/core/lib/Drupal/Core/Ajax/AjaxSubscriber.php
    deleted file mode 100644
    

    \o/

  2. +++ b/core/lib/Drupal/Core/EventSubscriber/ContentControllerSubscriber.php
    @@ -21,39 +20,6 @@
    -  public function onRequestDeriveFormat(GetResponseEvent $event) {
    -    $request = $event->getRequest();
    -
    -    if (!$request->attributes->get('_format')) {
    -      $request->setRequestFormat($this->negotiation->getContentType($request));
    -    }
    -  }
    

    \o/

  3. +++ b/core/lib/Drupal/Core/StackMiddleware/NegotiationMiddleware.php
    @@ -0,0 +1,80 @@
    +   * The content negotiation.
    ...
    +   *   The content negotiation.
    

    negotiator

    (because the property is named that way)

  4. +++ b/core/lib/Drupal/Core/StackMiddleware/NegotiationMiddleware.php
    @@ -0,0 +1,80 @@
    +  /**
    +   *
    +   * @var array
    +   */
    +  protected $formats;
    

    Incomplete docs.

  5. +++ b/core/lib/Drupal/Core/StackMiddleware/NegotiationMiddleware.php
    @@ -0,0 +1,80 @@
    +   * Registers a format for a given mime type.
    ...
    +   *   The mime type.
    

    s/mime/MIME/

    ?

  6. +++ b/core/modules/hal/src/HalServiceProvider.php
    @@ -0,0 +1,28 @@
    diff --git a/core/modules/hal/src/HalSubscriber.php b/core/modules/hal/src/HalSubscriber.php
    
    diff --git a/core/modules/hal/src/HalSubscriber.php b/core/modules/hal/src/HalSubscriber.php
    deleted file mode 100644
    

    \o/

  7. +++ b/core/modules/system/src/Tests/Bootstrap/PageCacheTest.php
    @@ -98,6 +99,50 @@ function testAcceptHeaderRequests() {
    +    // Clear the page cache and check first with a HAL request and then an
    +    // ordinary html on.
    

    "and then an ordinary html on"

    -> cannot parse that :)

znerol’s picture

+++ b/core/lib/Drupal/Core/StackMiddleware/NegotiationMiddleware.php
@@ -0,0 +1,80 @@
+/**
+ * Provides a middleware to determine the content type upon the accept header.
+ */
+class NegotiationMiddleware implements HttpKernelInterface {

Please add a @todo here, the middleware is temporary and needs to be removed ASAP again.

dawehner’s picture

Issue summary: View changes
FileSize
34.62 KB
2.57 KB

Done.

Fabianx’s picture

RTBC + 1

Wim Leers’s picture

Another quick reroll for #20?

neclimdul’s picture

+++ b/core/lib/Drupal/Core/StackMiddleware/NegotiationMiddleware.php
@@ -14,6 +14,8 @@
+ * @todo This is a temporary solution, remove this in https://www.drupal.org/node/2364011

Its there

Wim Leers’s picture

Oh, oops :) I overlooked that. Excellent :)

Crell’s picture

Issue tags: +WSCCI

Tagging.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committing 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!

  • alexpott committed 13e0794 on 8.0.x
    Issue #2445723 by dawehner, neclimdul: Use the $request format instead...

Status: Fixed » Closed (fixed)

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

andypost’s picture