Problem/Motivation

Currently, we add an X-Generator header with value "Drupal" to HTML pages. This is useful as it helps market that Drupal is everywhere. However, we do that in the HtmlRenderer service. (As of #2463009: Introduce CacheableResponseInterface: consolidate ways of setting X-Drupal-Cache-Tags/Contexts headers, anyway.) That has 2 problems:

1) It doesn't get added to non-HTML requests, which is still a minority but an increasing minority of requests. We should add it globally.
2) It's difficult to remove, if someone wants to disable that header for some reason. (Some argue that identifying the site as Drupal is a security risk; that's not debatable in this issue, but people should be able to opt-out.)

Proposed resolution

This is a self-contained bit of functionality, so let's make it so. Move the header addition from HtmlRenderer to its own dedicated Response subscriber. That listener should add the appropriate header (the code that's currently in HtmlRenderer, nearly verbatim) to any master request-based response that it sees, and that's all it does.

That cleans up HtmlRenderer a bit, and also makes it easy to disable by simply disabling that subscriber via a compiler pass. (Writing such a pass is out of scope for this issue.)

Remaining tasks

Do it!

User interface changes

None.

API changes

None

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because responses from Drupal are supposed to have the X-Generator header.
Issue priority Normal because it doesn't block anything.
Disruption Should be no disruption to anyone, unless some REST client is currently relying on there being no X-Generator header. Any such client deserves to break. :-)
CommentFileSizeAuthor
#38 move_x_generator_header-2477461-38.patch5.38 KBborisson_
#38 interdiff.txt2.45 KBborisson_
#30 interdiff.txt1.76 KBborisson_
#30 move_x_generator_header-2477461-30.patch2.92 KBborisson_
#25 move_x_generator_header-2477461-25.patch2.96 KBborisson_
#25 interdiff.txt601 bytesborisson_
#21 move_x_generator_header-2477461-21.patch3.1 KBborisson_
#21 interdiff.txt922 bytesborisson_
#19 move_x_generator_header-2477461-18.patch3.22 KBborisson_
#11 move_x_generator_header-2477461-11.patch3.26 KBgoogletorp
#6 move_x_generator_header-2477461-6.patch3.22 KBborisson_
#6 interdiff.txt1.92 KBborisson_
#4 move_x_generator_header-2477461-4.patch3.25 KBborisson_
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

Why not just add it to FinishResponseSubscriber?

Crell’s picture

Crell CreditAttribution: Crell at Palantir.net commented
Issue summary: View changes
Crell’s picture

Crell CreditAttribution: Crell at Palantir.net commented

We could, but then to disable it you'd have to add another listener and do a header->remove() call. I figured the better isolation was a good thing, especially since I would like FinishResponseSubscriber to go away eventually. :-) I dislike dumping ground services.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ at iO commented
Assigned: Unassigned » borisson_
FileSize
move_x_generator_header-2477461-4.patch3.25 KB

Should there be a test that checks if the X-Generator is present? I guess this can be added to any test that checks wether certain headers are set so this shouldn't need a new test.

Not setting to needs review because this won't apply without #2463009 being in head.

Crell’s picture

Crell CreditAttribution: Crell at Palantir.net commented 
  1. +++ b/core/core.services.yml
@@ -967,6 +967,11 @@ services:
+  finish_response_add_generator:

    This isn't part of finish_response. Call it "response_generator_subscriber" for consistency. 

  2. +++ b/core/lib/Drupal/Core/EventSubscriber/AddGenereratorResponseSubscriber.php
@@ -0,0 +1,47 @@
+    $events[KernelEvents::RESPONSE][] = array('onRespond');

    Let's go ahead and use all short-array-syntax. Mixing it is confusing and core is slowly moving toward short-arrays.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ at iO commented
FileSize
interdiff.txt1.92 KB
move_x_generator_header-2477461-6.patch3.22 KB

Fixed the remarks from #5.

googletorp’s picture

googletorp CreditAttribution: googletorp at Reveal IT commented
Status: Active » Needs review

Updated status to trigger test bot

The last submitted patch, 4: move_x_generator_header-2477461-4.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 6: move_x_generator_header-2477461-6.patch, failed testing.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ at iO commented

Test bot will fail as long as 2463009 is not commited. Setting the issue back to NW.

googletorp’s picture

googletorp CreditAttribution: googletorp at Reveal IT commented
FileSize
move_x_generator_header-2477461-11.patch3.26 KB

Reroll the patch, since it doesn't apply to head atm.

borisson_ queued 6: move_x_generator_header-2477461-6.patch for re-testing.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 11: move_x_generator_header-2477461-11.patch, failed testing.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ at iO commented
Status: Needs work » Needs review

The patch in #6 applies now that #2463009: Introduce CacheableResponseInterface: consolidate ways of setting X-Drupal-Cache-Tags/Contexts headers is in.

Crell queued 11: move_x_generator_header-2477461-11.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 11: move_x_generator_header-2477461-11.patch, failed testing.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Issue tags: +Needs reroll
borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ at iO commented
Status: Needs work » Needs review
FileSize
move_x_generator_header-2477461-18.patch3.22 KB

Reuploaded the same patch as in #6.
This patch should apply. as it was intended to be applied after #2463009: Introduce CacheableResponseInterface: consolidate ways of setting X-Drupal-Cache-Tags/Contexts headers.

The reroll @Googletorp did in #11 should doesn't apply anymore because it doesn't take the changes from #2463009: Introduce CacheableResponseInterface: consolidate ways of setting X-Drupal-Cache-Tags/Contexts headers into account.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs review » Needs work
Issue tags: -Needs reroll

@borisson_++

  1. +++ b/core/lib/Drupal/Core/EventSubscriber/ResponseGeneratorSubscriber.php
@@ -0,0 +1,47 @@
+ * Definition of Drupal\Core\EventSubscriber\ResponseGeneratorSubscriber.

    s/Definition of D/Contains \D/ 

  2. +++ b/core/lib/Drupal/Core/EventSubscriber/ResponseGeneratorSubscriber.php
@@ -0,0 +1,47 @@
+   * Sets extra X-Generator header on successful responses.

    Only successful responses? 

  3. +++ b/core/lib/Drupal/Core/EventSubscriber/ResponseGeneratorSubscriber.php
@@ -0,0 +1,47 @@
+  /**
+   * Registers the methods in this class that should be listeners.
+   *
+   * @return array
+   *   An array of event listener definitions.

    Can be replaced with an @inheritdoc.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ at iO commented
FileSize
interdiff.txt922 bytes
move_x_generator_header-2477461-21.patch3.1 KB

I fixed 20.1 and 20.3.
Regarding 20.2: I copied this from the finishResponseSubscriber where the onRespond method has "Sets extra headers on successful responses." as docblock, to keep this one in line with the other Subscriber that's why I chose this as description. I don't see how I can test this on a insuccessful response.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented
Status: Needs work » Needs review
googletorp’s picture

googletorp CreditAttribution: googletorp at Reveal IT commented
Status: Needs review » Reviewed & tested by the community

@borisson_ Sorry for messing up the patch reroll, didn't see it depending on the other issue.

Patch looks good to me.

Crell’s picture

Crell CreditAttribution: Crell at Palantir.net commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/core.services.yml
@@ -967,6 +967,11 @@ services:
+    arguments: ['@language_manager', '@config.factory', '@page_cache_request_policy', '@page_cache_response_policy', '@cache_contexts_manager']

Actually, this line is unnecessary. The class has no constructor, so passing anything in arguments is redundant.

It looks like it was just copied from the service above, which does use/need those.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ at iO commented
Status: Needs work » Needs review
FileSize
interdiff.txt601 bytes
move_x_generator_header-2477461-25.patch2.96 KB

It was a copy & paste from the service above, removed the unneeded dependencies.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
Status: Needs review » Reviewed & tested by the community

Very nice!

Crell’s picture

Crell CreditAttribution: Crell at Palantir.net commented

+1 (Subscribe!)

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

Sorry, two more nits. But both can be fixed on commit :)

  1. +++ b/core/lib/Drupal/Core/EventSubscriber/ResponseGeneratorSubscriber.php
@@ -0,0 +1,44 @@
+ * Contains Drupal\Core\EventSubscriber\ResponseGeneratorSubscriber.

    Still missing leading backslash. 

  2. +++ b/core/lib/Drupal/Core/EventSubscriber/ResponseGeneratorSubscriber.php
@@ -0,0 +1,44 @@
+  }
+}

    Should have newline in between.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Reviewed & tested by the community » Needs work

#2478443: Set the 'is-active' class for anonymous users in a Response Filter instead of a #post_render_cache callback just landed, this needs a quick reroll, sorry!

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ at iO commented
Status: Needs work » Needs review
FileSize
move_x_generator_header-2477461-30.patch2.92 KB
interdiff.txt1.76 KB

Fixed both nitpicks from #28 and rerolled the patch.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs review » Reviewed & tested by the community

Looks perfect for me!

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

+1, looks perfect.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 30: move_x_generator_header-2477461-30.patch, failed testing.

cosmicdreams queued 30: move_x_generator_header-2477461-30.patch for re-testing.

cosmicdreams’s picture

cosmicdreams CreditAttribution: cosmicdreams commented

It doesn't make sense why there were so many fails.

Fabianx’s picture

Fabianx CreditAttribution: Fabianx for Acquia commented
Status: Needs work » Reviewed & tested by the community

Back to RTBC

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

Afaics this is not tested anywhere - I think we should add test to confirm this is added to html (so we know we've not broken anything) and a non-html (to know that we've had the desired effect).

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ at iO commented
Status: Needs work » Needs review
FileSize
interdiff.txt2.45 KB
move_x_generator_header-2477461-38.patch5.38 KB

I added a test that checks if the header is correctly added for a normal response (drupalGet for created node), a 404 reponse and a 200 response trough the rest module.

I'm not sure about the location of the test. I extended the test from RESTTestBase, so that the testing for the rest module was easy to implement, not sure if that's the ideal solution either.

Fabianx’s picture

Fabianx CreditAttribution: Fabianx for Acquia commented
Status: Needs review » Reviewed & tested by the community

It looks good to me.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Component: request processing system » rest.module
Category: Task » Bug report
Priority: Normal » Minor
Issue summary: View changes
Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs tests

I think that this is a bug against the rest module. Drupal responses are suppose to have the X-Generator header. The patch here fixes that and tests it (thanks).

Given that this is a bug and the change is not disruptive as per beta evaluation in the issue summary - this is good to go. Committed c21e099 and pushed to 8.0.x. Thanks!

  • alexpott committed c21e099 on 8.0.x 
    Issue #2477461 by borisson_, googletorp, Wim Leers, Crell: Move X-...

Status: Fixed » Closed (fixed)

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