Rename & document Drupal\Core\EventSubscriber\AcceptNegotiation406 KernelEvents::VIEW event subscriber

All good here.

So the question is - what needed fixing?

To be honest I would first try to actually throw away this class and see whether it is still needed.

NW for #5.

EntityResourceTestBase and its child classes still testing the Accept header. Does rest still provide support for Accept Header or we only provide support of _format query parameter?

//Naveen

#10: no, it's not testing that. That test coverage is sending the Accept request header in some cases only because of edge cases in Drupal's/Symfony's PHP exception handling. We're fixing those edge cases, which may allow us to remove those.

See #2842465: Behavior of fatal PHP errors/uncaught exceptions results is mostly undefined: \Drupal\Core\EventSubscriber\DefaultExceptionSubscriber::onJson() is the de facto catch-all.

Looking into this myself now.

So, this is still necessary for the case of:

1. controller returns render array
2. ?_format= is set to something other than html
3. hence there is no KernelEvents::VIEW event subscriber (see https://www.drupal.org/docs/8/api/render-api/the-drupal-8-render-pipeline for more about such event subscribers) to transform the render array to the requested format, because render arrays can only be transformed to the HTML format
4. this \Drupal\Core\EventSubscriber\AcceptNegotiation406 VIEW event subscriber detects that, and throws a 406 response, because the requested format is not acceptable

Conclusion: the problems are:

1. we still need this class
2. test coverage is missing if the only failing tests are REST tests — there must be render system functional test that ensures this response is what you get when you request a controller that returns a render array in a non-HTML format
3. this event subscriber is poorly named, RenderArrayNonHtmlSubscriber would be a better name
4. the class docblock is also misleading
5. the overall documentation is misleading

thanks for the insight. Here's the initial patch without tests. We need new tests to ensure this functionality.Will write tests once the bot will be happy with these changes.

// Naveen

#15: that's looking great already! Thanks :)

+++ b/core/lib/Drupal/Core/EventSubscriber/RenderArrayNonHtmlSubscriber.php
@@ -8,19 +8,19 @@
+   * Throws an HTTP 406 error if a controller request in non-HTMl format.

s/if …/when a render array controller result must be converted to a format other than HTML/

Ignore #17, I like #16.1 better :)

1. +++ b/core/core.services.yml
   @@ -1039,8 +1039,8 @@ services:
   -  accept_negotiation_406:
   -    class: Drupal\Core\EventSubscriber\AcceptNegotiation406
   +  renderer_non_html:
   +    class: Drupal\Core\EventSubscriber\RenderArrayNonHtmlSubscriber
        tags:
          - { name: event_subscriber }
      main_content_renderer.html:
   

   If we rename a service, we need a container rebuilding, aka. an empty update method.

2. +++ b/core/lib/Drupal/Core/EventSubscriber/RenderArrayNonHtmlSubscriber.php
   @@ -8,19 +8,19 @@
   + * Provides a subscriber to return a 406 response when a request call a
   + * controller that returns a render array in a non-HTML format.
   

   ... And nothing converted it in the meantime. Maybe we should document it.

3. +++ b/core/lib/Drupal/Core/EventSubscriber/RenderArrayNonHtmlSubscriber.php
   @@ -8,19 +8,19 @@
   +   * Throws an HTTP 406 error if a controller request in non-HTMl format.
   

   Let's better use HTML instead of HTMl :)

In addition to #16 and #19,

1.  /**
   - * View subscriber rendering a 406 if we could not route or render a request.
   + * Provides a subscriber to return a 406 response when a request call a
   + * controller that returns a render array in a non-HTML format.
     *
   - * @todo fix or replace this in https://www.drupal.org/node/2364011
     */
   -class AcceptNegotiation406 implements EventSubscriberInterface {
   +class RenderArrayNonHtmlSubscriber implements EventSubscriberInterface {
   

   Class summary should fit on a single line.
   

2. -        // @see \Drupal\Core\EventSubscriber\AcceptNegotiation406::onViewDetect406()
   +        // @see \Drupal\Core\EventSubscriber\RenderArrayNonHtmlSubscriber::onViewDetect406()
   

   That method onViewDetect406() is renamed in this patch.

Rewriting documentation to hopefully make it more clear and adding empty update hook. Still needs tests.

Thanks! Setting to "needs review" so testbot can start the testing :)

+++ b/core/lib/Drupal/Core/EventSubscriber/RenderArrayNonHtmlSubscriber.php
@@ -0,0 +1,44 @@
+ * View subscriber throwing a 406 when response is not in requested format.

Well, this documentation should talk about HTML, not about some generic case.

#25: agreed, and I provided a suggestion in #17.

Finally, it'd be good to create this patch with git diff -M5%, to record it as a rename/move, instead of a file deletion + creation.

Uploading a patch made with git diff -M5% and with updated class summary string.

Both variants

/**
 * View subscriber throwing a 406 when a render array controller result must be
 * converted to a format other than HTML.
 */
class RenderArrayNonHtmlSubscriber implements EventSubscriberInterface {

and

/**
 * Throws an HTTP 406 error when a render array controller result must be
 * converted to a format other than HTML.
 */
class RenderArrayNonHtmlSubscriber implements EventSubscriberInterface {

are too long and do not fit on a single line as the coding standards require. Can this requirement be overlooked this time?

Thanks, that's much better!

1. Regarding the class docblock:

   Throws 406 if requesting non-HTML format and controller returns render array.
   

   That fits, and again takes a cue from @vijaycs85's excellent suggestion in #16.1.

2. +++ b/core/lib/Drupal/Core/EventSubscriber/RenderArrayNonHtmlSubscriber.php
   @@ -8,26 +8,27 @@
   -    // not HTML though we can also assume that the requested format is invalid
   -    // so we provide a 406 response.
   +    // not HTML though, we can also assume the following:
   +    //   - HTML resulting from a render array is invalid for requested format;
   +    //   - result was not converted by anything else;
   +    //   - it is better to return 406 error.
   

   I think it's better to revert this change; the original comment was clearer.

3. +++ b/core/modules/system/system.install
   @@ -1811,3 +1811,19 @@ function system_update_8301() {
   + * Add detailed cron logging configuration.
   

   The 'accept_negotiation_406' service was renamed.

4. +++ b/core/modules/system/system.install
   @@ -1811,3 +1811,19 @@ function system_update_8301() {
   +  // Empty update to cause a cache rebuild.
   

   s/cache/container/

Addressing feedback from #28, thanks @Wim Leers! I should stay away from code comments :)

Still needs tests.

This looks perfect now other than it needs tests :)

The missing test coverage:

[…] there must be render system functional test that ensures this response is what you get when you request a controller that returns a render array in a non-HTML format

If you want, I can give additional pointers.

I wasn't sure where is the best place to put these tests, I think in system module is good.

Test only patch should not have passed...

No, they should have passed after all. The tested code already exists in core and we simple rename it here. And I'm adding a missing test coverage here that should be passing.

#33: exactly! :)

However, you can prove that your tests are testing the right thing by deleting (or commenting) this line:

$events[KernelEvents::VIEW][] = ['onRespond', -10];

That will cause the event subscriber to not run. And should then result in failing tests. If the tests do not fail, then the test are not testing what they should be testing :)

1. +++ b/core/modules/system/tests/modules/render_array_non_html_subscriber_test/src/RenderArrayNonHtmlSubscriberTestController.php
   @@ -0,0 +1,28 @@
   +  public function rawStrign() {
   

   s/Strign/String/

2. +++ b/core/modules/system/tests/src/Functional/Render/RenderArrayNonHtmlSubscriberTest.php
   @@ -0,0 +1,61 @@
   +  /**
   +   * Tests that event subscriber does not interfere with normal requests.
   +   */
   +  public function testRenderedResponse() {
   +    $url = Url::fromRoute('render_array_non_html_subscriber_test.render_array');
   +
   +    $this->drupalGet($url);
   +    $this->assertSession()->statusCodeEquals(200);
   +    $this->assertRaw(t('Controller response successfully rendered.'));
   +  }
   +
   +  /**
   +   * Tests that correct response code is returned for any non-HTML format.
   +   */
   +  public function testNonHtmlRequestFormat() {
   +    foreach (['json', 'hal+json', 'xml', 'foo'] as $format) {
   +      $url = Url::fromRoute('render_array_non_html_subscriber_test.render_array', [
   +        '_format' => $format,
   +      ]);
   +
   +      $this->drupalGet($url);
   +      $this->assertSession()->statusCodeEquals(406);
   +      $this->assertNoRaw(t('Controller response successfully rendered.'));
   +    }
   +  }
   +
   +  /**
   +   * Tests that event subscriber does not interfere with raw string responses.
   +   */
   +  public function testRawStringResponse() {
   +    $url = Url::fromRoute('render_array_non_html_subscriber_test.raw_string', [
   +      '_format' => 'foo',
   +    ]);
   +
   +    $this->drupalGet($url);
   +    $this->assertSession()->statusCodeEquals(200);
   +    $this->assertRaw(t('Raw controller response.'));
   +  }
   

   We can merge these into a single testBlah() method. No need to install Drupal three times.

Primarily NW for creating such a failing patch!

Oh, and after that, this is RTBC :)

Right, there are not unit tests, so they have some setup overhead. Thanks for the reviews, @Wim Leers!

Now it should be failing as expected.

Perfect! :)

P.S.: tip for the future: upload the failing patch first. Then the issue won't be marked NW. :)

Got it. Thanks!

+++ b/core/modules/system/system.install
@@ -1834,3 +1834,19 @@ function system_update_8301() {
+function system_update_8400() {
+  // Empty update to cause a container rebuild.
+}
+

Don't we do post updates or something for this now?

+++ b/core/lib/Drupal/Core/EventSubscriber/RenderArrayNonHtmlSubscriber.php
@@ -8,25 +8,23 @@
-class AcceptNegotiation406 implements EventSubscriberInterface {
+class RenderArrayNonHtmlSubscriber implements EventSubscriberInterface {
...
-  public function onViewDetect406(GetResponseForControllerResultEvent $event) {
+  public function onRespond(GetResponseForControllerResultEvent $event) {

Just a note: These changes are considered backwards-compatible because event subscribers, services, etc. are considered internal: https://www.drupal.org/core/d8-bc-policy#paramconverters

However, since it's an internal API change, this issue is correctly filed as an 8.4.x-only (minor version) issue.

Just basically confirming all is well here since I had to double-check.

Mostly not a docs patch.

Yeah I don't really see an advantage of adding this to 8.3.x, at least not for before 8.3.0

+1, this is pure clean-up.

See question in #41 though. Thanks!

Closed #2725435: Remove outdated @todo pointing to #2364011 as duplicate.

IDK the answer to #41.

18:49:41 <WimLeers> dawehner: catch Do we nowadays use hook_update or hook_post_update if a change simply requires a container rebuild? xjm is asking at https://www.drupal.org/node/2835626#comment-11930278, but IDK the answer. https://www.drupal.org/node/2563673 definitely doesn't mention this

I would use a hook_update_N(), as the system is not in a working/consistent state, unless the container is rebuild. hook_post_update_X assumes to have a fully working environment.

Given #49…

+++ b/core/modules/system/system.install
@@ -1834,3 +1834,19 @@ function system_update_8301() {
+/**
+ *  The 'accept_negotiation_406' service was renamed.
+ */
+function system_update_8400() {
+  // Empty update to cause a container rebuild.
+}

This is a bit of an odd one - I'm not sure this is necessary. A container rebuild is guaranteed when you update from 8.3.0 (or whatever release you are on) to 8.4.0 since the container cache key changes. I'll ping @dawehner about this given it was due to #19 we added this. I'm unsure.

I guess we don't care about oeople running dev versions? I'm talking about people using git, as they might pull a specific commit in using composer.

If you're running a dev version, you can use $settings['deployment_identifier']. Part of why we added that stuff in the first place was to stop sites having fatal errors before updates had even been run.

Also 8.4.x already has an update committed to it.

So from #53 @catch is saying to remove the update hook implementation. Thanks!

Here is the new patch,

Even better :)

If you're running a dev version, you can use $settings['deployment_identifier'].

To be honest though, nobody really knows about that.

@dawehner Yeah! I just came to know about this from this issue. Maybe this is because i am new to Drupal :)

If you're running a dev version, you can use $settings['deployment_identifier'].

To be honest though, nobody really knows about that.

Maybe we should make a followup to fix that in the docs?

So while this BC break is allowed as an internal change, the language in our policy that allows it is definitely in the fine print. So it occurs to me now that we should add a change record for this. Sorry I didn't think of that sooner!

CR created: https://www.drupal.org/node/2856658

Needs a re-roll.

Re-rolled.

Thanks!

Now passing fine, suspect a testbot glitch. Returning to RTBC.

DrupalCI infra fail.

Random fail in Drupal\Tests\views\FunctionalJavascript\Plugin\views\Handler\ContextualFilterTest.

Sigh.

Fixed this on commit, and committed/pushed to 8.4.x, thanks!

FILE: ...lib/Drupal/Core/EventSubscriber/RenderArrayNonHtmlSubscriber.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 37 | ERROR | [x] Visibility must be declared on method
    |       |     "getSubscribedEvents"
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Published CR: https://www.drupal.org/node/2856658.