Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
- #1937600: Determine what services to register in the new Drupal class adds methods to the
Drupal
class for services that are intended as public APIs. That issue originally proposed adding therequest
service. - @Crell replied with this feedback:
Is ::request() intended to be the current request object? While I know that's common, I'd almost suggest we don't expose that specifically to discourage its use. The more code we have that's request-agnostic, the more easily we'll be able to break code up and unit test it (and run it through drush without weird contortions).
- Others have disagreed.
-
#1847768: Remove ip_address() has lots of changes like this:
+++ b/core/includes/session.incundefined @@ -173,7 +173,7 @@ function _drupal_session_write($sid, $value) { - 'hostname' => ip_address(), + 'hostname' => Drupal::service('request')->getClientIP(),
While reviewing that issue, @webchick suggested that this should be cleaned up.
Proposed resolution
- Discuss whether or not to add
Drupal::request()
. - If appropriate:
- Add
Drupal::request()
. - Convert existing uses of
Drupal::service('request')
anddrupal_container()->get('request')
.
- Add
Comment | File | Size | Author |
---|---|---|---|
#16 | 1957156-Drupal-request-16.patch | 1.5 KB | ParisLiakos |
#16 | interdiff.txt | 427 bytes | ParisLiakos |
#5 | 1957156-Drupal-request.patch | 1.49 KB | Crell |
Comments
Comment #1
Damien Tournoud CreditAttribution: Damien Tournoud commentedI think this is actually a good reason to add
::request()
: the docblock of this method is the best place we have to explain why you should preferably not use it.Comment #2
Crell CreditAttribution: Crell commentedThat's... hm. A valid point. I'll have to think on that, but depending on how we phrase it that might be useful.
Comment #3
xjmI like that idea as well.
Comment #4
msonnabaum CreditAttribution: msonnabaum commentedWe should absolutely have this. It's a lot to ask for people to go from something like:
to
Having this is a decent compromise:
Comment #5
Crell CreditAttribution: Crell commentedSo, something like this.
Comment #6
msonnabaum CreditAttribution: msonnabaum commentedIt's perhaps more discouraging than necessary, but anyone who doesn't care can easily ignore it, so I'm good.
Comment #7
tstoecklerI won't un-RTBC, but: I very much like the thorough description, but I think the second paragraph can be dropped entirely. While accurate, it is more a description of D8 programming patterns in general and not specific to \Drupal::request(). Such a paragraph should be at the top of \Drupal itself, but not on this method.
Comment #8
Damien Tournoud CreditAttribution: Damien Tournoud commented@tstoeckler: The use
Drupal::
as a whole should be discouraged, as it is an infringement to the dependency injection pattern. But the use ofDrupal::request()
is even worse: if you use this, you are *most probably* doing it wrong. Such a direct use of the request object has consequences on the whole stack: block caching, page caching, sub-requests (ie. ESI/SSI), command line tools (Drush and the like), queue processors, etc.Comment #9
webchickRight, I think that's what he's saying. The entire Drupal class is only a shim because, barring the core development team swelling by 3x the amount of Very Smart People working on it from now until July 1, we're never going to get all the way to OO everywhere in Drupal 8. It's just not going to happen.
I think probably the best thing to do is remove the discouraging wording here, and open a follow-up for how to explain the "Drupal" class as a whole and when it should / should not be used, and what viable alternatives there are.
Comment #10
webchickAlso,
public function request() {
should bepublic static function request() {
to match with everything else in the file, no?Comment #11
effulgentsia CreditAttribution: effulgentsia commentedI don't understand this objection. During a subrequest, Drupal::request() will return the $request object of the subrequest.
Comment #12
Crell CreditAttribution: Crell commentedYes, it should be static. I'll reroll tonight if someone doesn't beat me to it.
webchick: As Damien noted in #1, the whole reason for having a Drupal::request() method (IMO) is to have an extra place to say "no, really, you shouldn't be doing this", while still allowing it to be done for code that hasn't moved over to the new model yet. The second paragraph specifically is explaining what the good alternative is.
We need to lean in to the transition, and try to drive it forward. Just because a lot of code won't be converted yet doesn't mean we shouldn't encourage its conversion. Just because we won't be "OO everywhere" (which is entirely true) doesn't mean we shouldn't still encourage people to work in a loosely coupled way. Remember, Drupal::request() is just so much syntactic sugar around a global. *insert usual arguments about globals here*
Comment #13
Crell CreditAttribution: Crell commentedAlso, the Drupal class itself already has a huge docblock explaining why services are better. We don't need a follow up for that. It's already there, with code samples.
Comment #14
Damien Tournoud CreditAttribution: Damien Tournoud commentedThat is perfectly correct. During a sub-request, you are going to get the sub-request. The Drupal class is a request-local class, as it exists in most web frameworks. There is nothing wrong with that in general.
The problem with the request object is different: if you start using the request objects "by the side", you are going to completely by-pass the whole pipeline. There is no way for the block-level and page-level caching to know what you used from the request. It also by-passes security checks. It is invalid in contexts where you don't have a request (command line tools, batch processes, etc.).
Most likely, if you are doing that, you are doing it wrong. It's *exactly* the same thing as accessing
$_POST
or$_GET
directly: there is no legitimate reason to do that, and doing so will have consequences.Comment #15
Damien Tournoud CreditAttribution: Damien Tournoud commentedDrupal 8 cannot ship as a monstrous hybrid between an aspect-oriented, convention over configuration framework (Drupal 7) and an object-oriented, configuration over convention web framework (Symfony). If we ship this, we are going to lose the current community (because most of the modules are going to have to be reinvented completely, because of the new plugin system, services registered in the inversion of control container, etc.; not to mention the large amount of scaffolding required in the new framework), and we are not going to gain traction from external developers (because nobody will want to touch this monster with a ten foot pole).
We should know better.
There is a big difference between services in general and the request in particular. You can have several versions of the "same" service active at the same time, but you only have one request. There is no need for dependency injection for the request because of this. As I explained above, there are specific issues to consider about the request object. I think we should keep the discouraging wording in there.
Comment #16
ParisLiakos CreditAttribution: ParisLiakos commentedfixed the static thingie
Comment #17
Crell CreditAttribution: Crell commentedWhile I appreciate the sentiment in #15, I don't think we can avoid a hybrid environment in Drupal 8. Not unless we also rip out all hooks for D8, which is not happening.
But we definitely should be making it clear what direction we're leaning, which is toward injected OO. Documenting the Drupal class as entirely a shim (it is), including examples of what to do instead (it does), and calling out request specifically as it's the most likely bit of global state people would want to hack in (as this patch does) does exactly that.
Comment #18
webchickOk, let's go with this for now, then.
Committed and pushed to 8.x.
Comment #19
jibranDiscussion is over I think.
Comment #20
jibranDiscussion is over I think.
Comment #21
jibranPlease ignore bad internet.
Comment #22
geerlingguy CreditAttribution: geerlingguy commentedIs there somewhere that
is being discussed? So far it seems there are many module contributors who are getting nervous about D8 because they're sometimes popping into these discussions and seeing a lot of intense discussion about how OO (or not) Drupal 8 will be, and whether things like hooks (the understanding of which has been probably the core of Drupal module development since at least 2005?) will be first-class citizens of D8 or red-headed step children...
In my periodic 'dipping my toes in the water' reading of the D8 issue queue, I've not seen a really good explanation of where we want to be for Drupal 8, and why we want to be there (besides 'testing all the things', which is definitely a worthy cause).
I think the argument I've seen here and there that pitches Drupal 8 as marginalizing hobbyist programmers gains more merit when we can't point to a decent explanation about simple things like why someone wouldn't want to use globals in general, especially in Drupal, since for a very long time, and in so many places, globals usage is encouraged and codified. Even a nice, simple blog post on why we want to make Drupal more OO in general, and a simple explanation/diagram of how services, etc. are better than hooks, would go a long way.
[Edit: my basic point: rather than make 'hobbyist' programmers—many of whom may someday become more ardent Drupallers in general—feel marginalized in D8 (which is still under development, I know, I know), I think we can have our cake and eat it too, if we can communicate sound software architecture principals through not only D8 core architecture, but also through more clarity as to the why's behind the code.]
Comment #23
Crell CreditAttribution: Crell commentedgeerlingguy: I agree entirely; the challenge is that such educational efforts are decidedly non-trivial, and the people most equipped to discuss and explain architectural changes are still in the process of making architectural changes. :-)
Comment #24
geerlingguy CreditAttribution: geerlingguy commented@Crell - I'm planning on writing at least one or two blog posts on the process of converting Honeypot to D8, and am trying to incorporate some reasoning as to why certain things are radically different in D8... can you (or anyone else here) point to any resources (on your blog, or elsewhere) that would help me in my explanations?
I know I've seen a few posts here and there in the past (especially w/r/t Drupal adopting Symfony), but can't find much with cursory searches.
Comment #26
kim.pepperSpelling mistake on discourged (discouraged).