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
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. :-) |
Comment | File | Size | Author |
---|---|---|---|
#38 | move_x_generator_header-2477461-38.patch | 5.38 KB | borisson_ |
#38 | interdiff.txt | 2.45 KB | borisson_ |
#30 | interdiff.txt | 1.76 KB | borisson_ |
#30 | move_x_generator_header-2477461-30.patch | 2.92 KB | borisson_ |
#25 | move_x_generator_header-2477461-25.patch | 2.96 KB | borisson_ |
Comments
Comment #1
Wim LeersWhy not just add it to
FinishResponseSubscriber
?Comment #2
Crell CreditAttribution: Crell at Palantir.net commentedComment #3
Crell CreditAttribution: Crell at Palantir.net commentedWe 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.
Comment #4
borisson_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.
Comment #5
Crell CreditAttribution: Crell at Palantir.net commentedThis isn't part of finish_response. Call it "response_generator_subscriber" for consistency.
Let's go ahead and use all short-array-syntax. Mixing it is confusing and core is slowly moving toward short-arrays.
Comment #6
borisson_Fixed the remarks from #5.
Comment #7
googletorp CreditAttribution: googletorp at Reveal IT commentedUpdated status to trigger test bot
Comment #10
borisson_Test bot will fail as long as 2463009 is not commited. Setting the issue back to NW.
Comment #11
googletorp CreditAttribution: googletorp at Reveal IT commentedReroll the patch, since it doesn't apply to head atm.
Comment #13
Wim LeersComment #15
borisson_The patch in #6 applies now that #2463009: Introduce CacheableResponseInterface: consolidate ways of setting X-Drupal-Cache-Tags/Contexts headers is in.
Comment #18
Wim LeersComment #19
borisson_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.
Comment #20
Wim Leers@borisson_++
s/Definition of D/Contains \D/
Only successful responses?
Can be replaced with an
@inheritdoc
.Comment #21
borisson_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.
Comment #22
jibranComment #23
googletorp CreditAttribution: googletorp at Reveal IT commented@borisson_ Sorry for messing up the patch reroll, didn't see it depending on the other issue.
Patch looks good to me.
Comment #24
Crell CreditAttribution: Crell at Palantir.net commentedActually, 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.
Comment #25
borisson_It was a copy & paste from the service above, removed the unneeded dependencies.
Comment #26
tim.plunkettVery nice!
Comment #27
Crell CreditAttribution: Crell at Palantir.net commented+1 (Subscribe!)
Comment #28
Wim LeersSorry, two more nits. But both can be fixed on commit :)
Still missing leading backslash.
Should have newline in between.
Comment #29
Wim Leers#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!
Comment #30
borisson_Fixed both nitpicks from #28 and rerolled the patch.
Comment #31
dawehnerLooks perfect for me!
Comment #32
Wim Leers+1, looks perfect.
Comment #35
cosmicdreams CreditAttribution: cosmicdreams commentedIt doesn't make sense why there were so many fails.
Comment #36
Fabianx CreditAttribution: Fabianx for Acquia commentedBack to RTBC
Comment #37
alexpottAfaics 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).
Comment #38
borisson_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.
Comment #39
Fabianx CreditAttribution: Fabianx for Acquia commentedIt looks good to me.
Comment #40
alexpottI 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!