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 the request 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:
    1. Add Drupal::request().
    2. Convert existing uses of Drupal::service('request') and drupal_container()->get('request').
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Damien Tournoud’s picture

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

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

Crell’s picture

That's... hm. A valid point. I'll have to think on that, but depending on how we phrase it that might be useful.

xjm’s picture

I like that idea as well.

msonnabaum’s picture

We should absolutely have this. It's a lot to ask for people to go from something like:

ip_address();

to

Drupal::service('request')->getClientIP();

Having this is a decent compromise:

Drupal::request()->getClientIP()
Crell’s picture

Status: Active » Needs review
FileSize
1.49 KB

So, something like this.

msonnabaum’s picture

Status: Needs review » Reviewed & tested by the community

It's perhaps more discouraging than necessary, but anyone who doesn't care can easily ignore it, so I'm good.

tstoeckler’s picture

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

Damien Tournoud’s picture

@tstoeckler: The use Drupal:: as a whole should be discouraged, as it is an infringement to the dependency injection pattern. But the use of Drupal::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.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

Right, 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.

webchick’s picture

Also, public function request() { should be public static function request() { to match with everything else in the file, no?

effulgentsia’s picture

Such a direct use of the request object has consequences on the whole stack: block caching, page caching, sub-requests (ie. ESI/SSI)

I don't understand this objection. During a subrequest, Drupal::request() will return the $request object of the subrequest.

Crell’s picture

Yes, 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*

Crell’s picture

Also, 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.

Damien Tournoud’s picture

Such a direct use of the request object has consequences on the whole stack: block caching, page caching, sub-requests (ie. ESI/SSI)

I don't understand this objection. During a subrequest, Drupal::request() will return the $request object of the subrequest.

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

Damien Tournoud’s picture

Right, 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.

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

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.

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.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
427 bytes
1.5 KB

fixed the static thingie

Crell’s picture

Status: Needs review » Reviewed & tested by the community

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

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Ok, let's go with this for now, then.

Committed and pushed to 8.x.

jibran’s picture

Title: Discuss whether to add a request() method to \Drupal » Add a request() method to \Drupal
Issue tags: -DX (Developer Experience)

Discussion is over I think.

jibran’s picture

Discussion is over I think.

jibran’s picture

Please ignore bad internet.

geerlingguy’s picture

Is there somewhere that

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.

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

Crell’s picture

geerlingguy: 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. :-)

geerlingguy’s picture

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

Status: Fixed » Closed (fixed)

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

kim.pepper’s picture

+++ b/core/lib/Drupal.phpundefined
@@ -122,6 +122,32 @@ public static function service($id) {
+   * Note: The use of this wrapper in particular is especially discourged. Most

Spelling mistake on discourged (discouraged).