Updated: Comment #N

Problem/Motivation

Since D8 is heavily stressing route names over paths, it is really common to need to know the route name of a given page during development.

Proposed resolution

Add a class to help find the route name, path, and route object of a request.

Remaining tasks

N/A

User interface changes

N/A

API changes

N/A

N/A

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

Status: Active » Needs review
FileSize
2.65 KB

debug(drupal_get_route_name()) will become your best friend.

tim.plunkett’s picture

.

dawehner’s picture

I would love to see an actual test on the render level. Given that the main content is rendered different than the rest of the page (in a subrequest) you might loose the actual route name in the main request later.

tim.plunkett’s picture

FileSize
5.62 KB
2.97 KB

Like this?

dawehner’s picture

Issue summary: View changes
FileSize
2.18 KB

Here is an alternative approach.

tim.plunkett’s picture

FileSize
5.2 KB
7.26 KB

I like this. I combined the two, so we can show off the usage.

Do you think this needs an interface, or a method directly on \Drupal::?

tim.plunkett’s picture

Title: Add a procedural helper to find the route machine name of a given request » Add a helper class to introspect a given request
Issue summary: View changes
dawehner’s picture

Even the code looks potentially better I vote to not require to use that object so we keep explicit support for symfony related attributes.

Status: Needs review » Needs work

The last submitted patch, 6: route-name-2103301-6.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review

6: route-name-2103301-6.patch queued for re-testing.

dawehner’s picture

Just a note, code that uses the route_name, route object or _system_path is certainly written in a not really decoupled way.

Berdir’s picture

6: route-name-2103301-6.patch queued for re-testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
5.17 KB
7.38 KB

Rerolled at made the API a bit easier to use by being able to pass in the request as optional argument.

Status: Needs review » Needs work

The last submitted patch, 13: request_info-2103301-13.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
7.39 KB
530 bytes

Ups.

Status: Needs review » Needs work

The last submitted patch, 15: request_info-2103301-14.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
7.85 KB

This should fix it.

damiankloip’s picture

FileSize
10.97 KB
4 KB
+++ b/core/lib/Drupal/Core/Routing/RequestInfo.php
@@ -0,0 +1,87 @@
+    $request = $request ?: $this->request;
...
+    $request ?: $this->request;

These are kind of broken. You will never be able to use the request object passed as a parameter.

If we're going to do this, let's add a quick unit test for that stuff.

dawehner’s picture

Oh wonderful!

dawehner’s picture

18: 2103301-18.patch queued for re-testing.

jibran’s picture

+++ b/core/lib/Drupal/Core/Routing/RequestInfo.php
@@ -0,0 +1,87 @@
+  public function setRequest(Request $request) {

This function should return $this. Don't ask me why ask @tim.plunkett. :)

damiankloip’s picture

FileSize
11.01 KB
522 bytes

I'm good with that.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

None of our setRequest methods does that at the moment. I don't really think such an object is ever going to be used as a chainable object as you can just set one thing anyway.

sun’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/lib/Drupal/Core/Controller/DialogController.php
@@ -34,16 +35,26 @@ class DialogController {
-  public function __construct(ControllerResolverInterface $controller_resolver, TitleResolverInterface $title_resolver) {
+  public function __construct(ControllerResolverInterface $controller_resolver, TitleResolverInterface $title_resolver, RequestInfo $request_info) {
...
+    $this->requestInfo = $request_info;

@@ -125,7 +136,7 @@ public function dialog(Request $request, $_content, $modal = FALSE) {
-        $route_name = $request->attributes->get(RouteObjectInterface::ROUTE_NAME);
+        $route_name = $this->requestInfo->getRouteName($request);

Hm. This looks a bit weird to me — I did not expect the RequestInfo instance to be state on controller objects (which can get out of sync).

Isn't the RequestInfo as contextual as the Request itself?

Shouldn't it be passed as argument into individual controller methods instead?

damiankloip’s picture

But RequestInfo just examines the Request that is passed in and returns the value based on that.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Yeah and the syntax:
- [setRequest, ['@?request=']]ensures it is always up to date.

xjm’s picture

Issue tags: +API addition

Edit: I was totally wrong. Never mind.

xjm’s picture

Priority: Normal » Major
Issue tags: +beta target, +Needs change record

Now I added this to the correct change record:
https://drupal.org/node/2203305
It will need to be updated with the new code if/when this patch lands.

Bumping to major beta target as this is an important DX issue. This seems like an improvement over what we have DX-wise, but is it the final DX we want? I'm open to iterating after this lands but it would be good to get a few more eyes on this.

I also discussed with @dawehner whether something similar would be useful for the replacement for menu_get_object() but that can be explored in a followup issue.

dawehner’s picture

One thing we could do on top of it, especially once we also cover some more direct replacement for menu_get_object() (which I disagree but this is another story)
is to allow people to easily inject the RequestInfo into their controllers, much like they can do with the request object now, so something like the following
could work:


class example {

  public function view(RequestInfo $request_info) {
  }

}

and no other work would be needed.

sun’s picture

Assigned: Unassigned » msonnabaum

#29 is essentially why I'm concerned about the approach taken in the proposed patch.

@msonnabaum and others discussed the need for a ContextBag to get injected into controllers a couple of times by now (replacing the Request argument in most cases), and the proposed change here appears to be a one-off incarnation of exactly that, but just for retrieving route info for a request only.

Given where we are in the release cycle, I still think this proposal could use some more architectural reviews.

xjm’s picture

Status: Reviewed & tested by the community » Needs review

Yeah, agreed I think (edit: that #29 is a concern and that we still could use architectural review). Let's be honest about the status then.

xjm’s picture

damiankloip’s picture

OK, I don't really agree with that. What else would live in this so called context bag? Either way, that would then not be what this issue was going for. So you might as well just close it now.

larowlan’s picture

FileSize
2.27 KB
12.26 KB

I think we should also add a method for getting the raw variables - as per attached patch

larowlan’s picture

FileSize
22.79 KB
32.57 KB

Starts moving some core stuff to use getRawVariables() method

Status: Needs review » Needs work

The last submitted patch, 35: request-info.35.patch, failed testing.

The last submitted patch, 35: request-info.35.patch, failed testing.

Crell’s picture

I can't say I'm a huge fan of this, but I don't have any concrete arguments against it either. :-) (If anything, this is an argument in favor of the FIG's currently-in-discussion Request/Response interfaces, which would make this sort of wrapping more transparent. That's a long way off though; just something I need to remember to bring up in those discussions.)

larowlan’s picture

Personally, I don't like that its a service.
I would prefer it was all static methods and the request argument was mandatory.
RequestInfo::getRawVariables($request) seems simpler than the whole DI dance and still gives us a central point through which all of it travels so changing _raw_variables to something else in a point release doesn't break any APIs.
I don't see any mention of static methods anywhere in this issue - were they discussed?

znerol’s picture

@larowlan: #2124749-14: [meta] Stop using $request->attributes->get(MAGIC_KEY) as a public API. I personally do not like static methods too much, but in this case that's probably a matter of taste.

Crell’s picture

static methods are just global functions with colons in the middle. We're trying to avoid global functions in the new code base, because they can't be tested.

larowlan’s picture

I figure it can be tested like so https://gist.github.com/larowlan/9339613
But I get that the dependency is hard-wired once you use a static.
But given how simple the methods are, and that the bulk of them take the request as an argument I still don't see the point in the object having state.

xtfer’s picture

static methods are just global functions with colons in the middle. We're trying to avoid global functions in the new code base, because they can't be tested.

Static methods are no more global than Class functions. That's a somewhat spurious argument.

I agree with the inference about testability, however I wonder whether there's much of a win for a service in this case, over the static method. Not that much, would be my opinion.

znerol’s picture

Let's try to come to a decision whether we want to introduce the request-info special case here in this issue or try to resolve the broader problem over at #2124749: [meta] Stop using $request->attributes->get(MAGIC_KEY) as a public API. I'm all for the second option. I strongly believe that we need to define a straight forward pattern (easy to understand and easy to test), such that it can be reused later in contrib.

effulgentsia’s picture

I think this would be better done as an adapter pattern with a static factory method. Something like this:

namespace Drupal\Core\Routing\Adapter;
class RoutingRequestAttributes {
  function __construct(ParameterBag $request_attributes) {
    $this->attributes = $request_attributes;
  }
  function getRouteName() {
    return $this->attributes->get(RouteObjectInterface::ROUTE_NAME);
  }
  static function adapt(ParameterBag $request_attributes) {
    return new static($request_attributes);
  }
}

And then consuming code would look like:

if (RoutingRequestAttributes::adapt($request->attributes)->getRouteName() == 'node.view') {

The reason why I think a static factory method here is acceptable is because we're already coupled to a constant that either is or should be defined in an interface (the above example uses one that is, and '_raw_variables' and '_system_path' should be declared as constants somewhere). Which means we're already locked into a particular API (i.e., an attribute in $request with a name matching an interface-defined constant). And all we need is an adapter to adapt that ugly API into something a little bit nicer for IDEs and documentation. While we could make the adapter factory into a service, I don't think doing so buys us anything. From a testing standpoint, I don't think the class being tested having a fixed dependency on an adapter class is any more of a burden than it having a fixed dependency on the interface that defines the constant.

If other components / contrib modules then come up with other internal attributes to dump into $request, then they can create similar adapters for the attributes they define.

Where I think a real service could come into play would be for use cases where the use of a request attribute can be kept entirely as a private implementation detail. For example, if the router service (or one of its friends) exposed methods like getRouteFromRequest(), and all consuming code could call that, leaving it up to the router to decide whether to store that info on $request->attributes, or somewhere else. But that's not the situation we currently have, and I don't think we should hold up this issue on trying to change Symfony CMF to work that way.

znerol’s picture

I like #45. In my opinion the fact that attributes are used is just an implementation detail and therefore I'd rather prefer using something like this:

if (RoutingRequest::adapt($request)->getRouteName() == 'node.view') {
dawehner’s picture

Status: Needs work » Needs review
FileSize
2.21 KB

Yeah, we need to accept that we will never allow to switch out the internals of the request object.

effulgentsia’s picture

@dawehner: would it be ok with you to just have a single static factory method, as in #45, rather than have each method as static and need a $request? if not, why not?

For naming, how about either RoutingRequestAttributes::adapt($request) or RoutingRequestContext::get($request)? I think it's helpful to have both the Routing prefix (since that's the only info that class is dealing with) and some suffix after Request, since it's not an entire request object, but just dealing with a portion of it.

Status: Needs review » Needs work

The last submitted patch, 47: request_context-2103301-47.patch, failed testing.

dawehner’s picture

Sure. There are a couple of questions: Do we realy want users to pass in the attributes given that this would reduce the potential DX win but on the other hand make it more generic.

For naming, how about either RoutingRequestAttributes::adapt($request) or RoutingRequestContext::get($request)? I think it's helpful to have both the Routing prefix (since that's the only info that class is dealing with) and some
suffix after Request, since it's not an entire request object, but just dealing with a portion of it.

Agreed. Let's go with the second one.

Crell’s picture

I like the one wrapping object more than the series of statics. I actually just wrote such a thing for Drupal 7 nodes for a client a week ago. IMO we should also wrap the request object, rather than just the attributes array. That way it's a bit more consistent, and if necessary we can also have utility methods for things other than the attributes array.

larowlan’s picture

On a related note, is there a reason why breadcrumb builders only get the attributes array and not the whole request, or at least a parameter bag?

dawehner’s picture

Status: Needs work » Needs review
FileSize
23.86 KB
16.97 KB

Replaced a couple of usecases.

Status: Needs review » Needs work

The last submitted patch, 53: request_context-2103301-52.patch, failed testing.

effulgentsia’s picture

  1. +++ b/core/lib/Drupal/Core/Routing/RoutingRequestContext.php
    @@ -0,0 +1,99 @@
    +namespace Drupal\Core\Routing;
    +class RoutingRequestContext {
    

    Thinking some more about naming, I don't like "Context". This class is very different than the Symfony\Component\Routing\RequestContext class. I'd like to resuggest "Attributes" as the best suffix. Counter to #46 and #51, I don't think "attributes" is "just an implementation detail". I think it's the entire meaning of this class. Everything except "attributes" in Symfony's Request object maps to an HTTP concept. Attributes is the only part of Request that is intentionally defined as the only place for non-HTTP-defined, framework-specific stuff.

    I also wonder if we can drop the "Routing" prefix and move this to the root of Drupal\Core (i.e., Drupal\Core\RequestAttributes). That's both less verbose, and saves us from having to decide whether things like _system_path and _authentication_provider are part of routing or not (I think arguments can be made for either position on each of those).

  2. +++ b/core/lib/Drupal/Core/Routing/RoutingRequestContext.php
    @@ -0,0 +1,99 @@
    +  public function __construct(Request $request) {
    +    $this->request = $request;
    +  }
    

    Per above, I think the constructor should take just the $attributes ParameterBag.

  3. +++ b/core/lib/Drupal/Core/Routing/RoutingRequestContext.php
    @@ -0,0 +1,99 @@
    +  public static function get(Request $request) {
    

    I agree with this taking $request. How about renaming the method to fromRequest() though?

  4. +++ b/core/lib/Drupal/Core/Routing/RoutingRequestContext.php
    @@ -0,0 +1,99 @@
    +  public function getParameter($name) {
    

    This could then be named get().

  5. +++ b/core/lib/Drupal/Core/Routing/RoutingRequestContext.php
    @@ -0,0 +1,99 @@
    +  public function getRouteObject() {
    

    Do we need the Object suffix? Routes are always objects, right?

  6. +++ b/core/modules/block/block.module
    @@ -119,7 +120,7 @@ function block_page_build(&$page) {
    -  if (\Drupal::request()->attributes->get(RouteObjectInterface::ROUTE_NAME) != 'block.admin_demo') {
    +  if (RoutingRequestContext::get(\Drupal::request())->getRouteName() != 'block.admin_demo') {
    

    Can we add a wrapper in \Drupal, to shorten this to \Drupal::requestAttributes()->getRouteName()?

znerol’s picture

I also wonder if we can drop the "Routing" prefix and move this to the root of Drupal\Core (i.e., Drupal\Core\RequestAttributes). That's both less verbose, and saves us from having to decide whether things like _system_path and _authentication_provider are part of routing or not (I think arguments can be made for either position on each of those).

No, that defeats the whole point of wrapping the request attributes altogether. The goal here should not be to create one big adapter with one method to access each possible attribute, but a pattern on how to design such an adapter object. This pattern should be reusable for other parts of core and contrib (e.g. replace $request->attributes->get('node'); with NodeRequestContext::get($request)->getNode();.

+++ b/core/lib/Drupal/Core/Routing/RoutingRequestContext.php
@@ -0,0 +1,99 @@
+  public function getParameter($name) {

Remove this method completely, this has no benefit over $request->attributes->get().

dawehner’s picture

FileSize
20.75 KB
22.14 KB

I also wonder if we can drop the "Routing" prefix and move this to the root of Drupal\Core (i.e., Drupal\Core\RequestAttributes). That's both less verbose, and saves us from having to decide whether things like _system_path and _authentication_provider are part of routing or not (I think arguments can be made for either position on each of those).

I would like to not expose information that this class is maybe just about attributes. If people know that already they can use the attributes already for themselves. What about using \Drupal\Core\RequestInfo ?
This is pretty short and gives you enough knowledge what the class might do.

Per above, I think the constructor should take just the $attributes ParameterBag.

Well, i kinda dislike that internal bit. Most code is dealing with the request object in its full glory already.

I agree with this taking $request. How about renaming the method to fromRequest() though?

Yeah, let's use the same method name as the RequestContext in symfony.

Do we need the Object suffix? Routes are always objects, right?

Well, if you just have getRoute() you can't be sure whether this is the route name or the route object. Therefore we do have routeName and routeObject methods.

Can we add a wrapper in \Drupal, to shorten this to \Drupal::requestAttributes()->getRouteName()?

Not sure whether this is a good idea in the first place. We want people to discourage to use the \Drupal shortcut but write OOP code which gets the request objects passed in.

dawehner’s picture

Status: Needs work » Needs review

.

The last submitted patch, 50: request_context-2103301-50.patch, failed testing.

effulgentsia’s picture

Not sure whether this is a good idea in the first place. We want people to discourage to use the \Drupal shortcut but write OOP code which gets the request objects passed in.

But we still have a lot of hook implementations. We could make \Drupal::requestAttributes() receive an optional $request to allow for situations where the caller actually has a passed-in $request, but a lot of our hooks don't have that.

No, that defeats the whole point of wrapping the request attributes altogether. The goal here should not be to create one big adapter with one method to access each possible attribute, but a pattern on how to design such an adapter object. This pattern should be reusable for other parts of core and contrib (e.g. replace $request->attributes->get('node'); with NodeRequestContext::get($request)->getNode();.

I agree with you about modules (both core and contrib) needing the ability to make their own adapters. What I'm wondering though is if there's much usefulness in having core/lib split the adapters by component, or if it's better DX to have a single adapter for all of core/lib. Because if we have a single one for all of core/lib, then it can have a shorter name, and a single method in \Drupal:: to get at it. Regardless of the core/lib decision, modules can still prefix with their module name to disambiguate.

I would like to not expose information that this class is maybe just about attributes. If people know that already they can use the attributes already for themselves. What about using \Drupal\Core\RequestInfo ?

I'll sit with RequestInfo for a bit and think about it. Though my first reaction to it is: what's the difference between Request and RequestInfo? I mean, Request itself is an object that contains information about a request, right? Whereas, I think "attributes" is more informative both in general terms and also if you know about the technical concept of a Symfony Request, while simultaneously being a regular, non-intimidating English word.

effulgentsia’s picture

Well, if you just have getRoute() you can't be sure whether this is the route name or the route object.

Yes you can, it's always the object. Just like in the rest of HEAD. And that's what the @return in the PHPDoc is for.

Crell’s picture

For hook implementations, I'm already telling people that a non-trivial hook should consist of nothing more than \Drupal::service('mymodule.whatever')->doThing(). The hook body shouldn't contain any meaningful code, because it is inherently not reusable. That's even documented on the \Drupal class, I think...

dawehner’s picture

@crell
Do you have an opinoin of getRoute() vs. getRouteObject()?

znerol’s picture

Given that there is already a static factory method on the object, would it be acceptable to make the $request-parameter optional and use \Drupal::request if it is missing?

Something like this:

/**
 * Gets a new instance of a request context.
 *
 * @param \Symfony\Component\HttpFoundation\Request $request
 *   (optional) The request to adapt. If not specified, the current request is
 *   retrieved from the service container.
 *
 * @return static
 *   An instance of the routing request context.
 */
public static function get(Request $request = NULL) {
  if (!isset($request)) {
    $request = \Drupal::request();
  }
  return new RequestInfo($request);
}
Crell’s picture

znerol: Totally not OK. We are trying to wean people off of "I always have access to the global request object", and any other global object. We should NOT make it easy to still access global state. Optional parameters that default to global state are the spawn of Satan. (Yes, I'm being slightly hyperbolic.)

dawehner: I believe the _route key in attributes is the route name, and _route_object is the loaded object. (In practice we're using the constants defined by the CMF Routing component, but those are the underlying keys, I think.) We should probably be consistent with that.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

const ROUTE_NAME = '_route';
const ROUTE_OBJECT = '_route_object';

So if we're going to have getRoute() be anything, it would be the name. But that is confusing. And the constants are explicit, we should be explicit too.

catch’s picture

Status: Reviewed & tested by the community » Needs review

Looks like an accidental RTBC?

effulgentsia’s picture

Patch looks great to me. Having slept on it, I'm ok with RequestInfo as a name. And getRouteObject() is fine too. Whether to add a method in \Drupal:: can be a normal priority follow up.

@znerol: the patch puts the adapter in \Drupal\Core. Given #60, are you ok with that?

If so, the only thing left before this can be RTBC is:

  1. +++ b/core/lib/Drupal/Core/RequestInfo.php
    @@ -0,0 +1,86 @@
    + * Contains \Drupal\Tests\Core\Routing\RequestContext.
    

    Not true.

  2. +++ b/core/lib/Drupal/Core/RequestInfo.php
    @@ -0,0 +1,86 @@
    + * Provides easy access to some routing related request information.
    

    s/routing/Drupal/

catch’s picture

+++ b/core/modules/block/lib/Drupal/block/Theme/AdminDemoNegotiator.php
@@ -8,6 +8,7 @@
 use Symfony\Cmf\Component\Routing\RouteObjectInterface;

Also looks like this could be removed?

znerol’s picture

@znerol: the patch puts the adapter in \Drupal\Core. Given #60, are you ok with that?

Sure.

effulgentsia’s picture

Status: Needs review » Needs work

Great. In that case, "needs work" for #68 and #69.

dawehner’s picture

Status: Needs work » Needs review
FileSize
20.81 KB
1.18 KB

Sure, here are the fixes for these bits.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

My RTBC in #66 was purposeful, because the patch contains the names I think make the most sense. Now that we've addressed #68, RTBC again.

Crell’s picture

I'm +1 here as well. There may be other methods to add to RequestInfo but those can be done in other issues as needed. Thanks, all!

jibran’s picture

I am already in love with RequestInfo class. So +1.

ParisLiakos’s picture

patch looks great +1 from me too.

I dont want to kick back from rtbc, but just a note

+++ b/core/lib/Drupal/Core/RequestInfo.php
@@ -0,0 +1,88 @@
+  public static function get(Request $request) {
+    return new RequestInfo($request);
+  }

I guess this method is for easier inlining than doing (new RequestInfo($request))->getSth()

But i find the name of the method inconsistent with the rest of the core. can we rename it to create() instead?
I think effulgentsia made more or less the same note in #55 naming it fromRequest() which is also better than get() imo

tim.plunkett’s picture

Reroll after #1316692: Convert hook_admin_paths() into declarative properties on routes, I didn't address #76. I think this is fine as is...

Dries’s picture

I really like this patch and tried to commit the patch in #72 but it failed. Waiting for #77 to turn green.

sun’s picture

I agree with previous commenters, get() is a very unusual and unexpected name for a static class instantiation method. get() normally means to retrieve a value from an already instantiated class.

If we want to stick with the "get" terminology, then getInstance().

fromRequest() or createFromRequest() (mirroring Request::createFromGlobals()) would be more appropriate. The less verbose fromRequest(), byRequest(), ofRequest() would also be fine. The construct should make sense in the scope and context of the calling code.

effulgentsia’s picture

I think get() is an ok name, because for any given $request object, whether get() returns a new instance or reuses an instance makes no difference; it's an implementation decision that the caller doesn't care about. We have a bunch of factories with get() as the method name for retrieving an instance. That RequestInfo has a static factory method rather than a separate factory class doesn't really change the appropriateness of that in my mind.

I think in calling context, RequestInfo::get($request) makes sense as "get the info object that's appropriate for $request". RequestInfo::fromRequest($request), as I suggested in #55, would be fine too, but it's longer, and I don't know that having the word "request" in the line 3 times instead of 2 helps disambiguate anything.

catch’s picture

Status: Reviewed & tested by the community » Needs review
RequestInfo::get($request)->getSystemPath()

Not sure why we need the double get($request)->getSomethingElse() here.

Also I'm not using this to get the request, I'm getting some random stuff that we put into the request object which shouldn't necessarily even be in there.

msonnabaum mention in irc a syntax like RequestInfo::getSystemPath($request); that's less verbose, you still pass the $request in rather than it being grabbed for you. Not sure wha'ts wrong with that.

effulgentsia’s picture

Would RequestInfo::of($request)->getSystemPath() be satisfactory? It would be the correct grammar: you're getting the info of the request.

I won't block RequestInfo::getSystemPath($request) if no one else does, but I don't think it's better. RequestInfo makes more sense to me as an object rather than just a place for all static methods. Think of it as an adapter object or facade object. OOP books and articles usually implement such things as objects, not entirely static classes. Why deviate from that? But ultimately, that's not a big sticking point for me.

ParisLiakos’s picture

We have a bunch of factories with get() as the method name for retrieving an instance.

Yes but those methods are not static. static factory methods that return the same class instantiated are usually called create() or createFromSomething()

znerol’s picture

Fully support #82. Keeping static out of the game also gives us the possibility of reusing the object (and keeping verbosity down when repeated access is necessary):

$requestInfo = RequestInfo::get($request);
if ($requestInfo->getRoutName() == 'route.66') {
  $route = $requestInfo->getRouteObject();
  // ...
}

It would also be acceptable to use adapt instead of get for the factory method. I don't think it is useful to enforce the createFromX convention.

dawehner’s picture

Yes but those methods are not static. static factory methods that return the same class instantiated are usually called create() or createFromSomething()

Well, these create methods thought are basically factories, which we don't really have here, given that we already just pass in everything.

It would also be acceptable to use adapt instead of get for the factory method. I don't think it is useful to enforce the createFromX convention.

Using "adapt" as methodname seems like an odd thing, given that this is not the most usual english word in code.

To be honest: Reusability is not that important, as it is kind of an edge case.

msonnabaum’s picture

RequestInfo makes more sense to me as an object rather than just a place for all static methods. Think of it as an adapter object or facade object. OOP books and articles usually implement such things as objects, not entirely static classes. Why deviate from that? But ultimately, that's not a big sticking point for me.

What is RequestInfo? Info about a Request? Isn't that just a Request? The case for this being a domain model breaks down quickly. This looks like classic feature-envy to me.

If this can't just be a replacement wrapper for Request for whatever unconvincing reason that's likely beyond the scope of this issue, we should just be realistic about the fact that these are functions that know how to get something from a Request object.

Crell’s picture

I agree with znerol here. Make it an object. It's an adapter object for a request. In an ideal world we would have a true-value-object request and a wrapping adapter that implements the same interface and adds utility methods. We're not in an ideal world, though. This is close enough, and minimizes the amount of "random functions that happen to have colons in them".

As for the name of the factory method, create() could be confusing since all of our other create() methods are passed the container. Beyond that, it's a bikeshed I'm not interested in painting.

msonnabaum’s picture

This is close enough, and minimizes the amount of "random functions that happen to have colons in them".

That's not an argument.

effulgentsia’s picture

What is RequestInfo? Info about a Request? Isn't that just a Request?

RequestInfo is in the \Drupal\Core namespace. We could name it DrupalRequestInfo, but that would be redundant with the namespace. In other words, it's info about a request that Drupal Core provides / cares about. That's not the same as a (Symfony) Request, which is an abstraction of HTTP. The Request object provides an API for the PHP framework/application code to get and set (non-HTTP-specified) derived information about the request. That API is $request->attributes->get() and $request->attributes->set(). We don't like that API so we need an adapter/facade around it.

Just as the Request object has multiple methods (e.g., getHost() and getClientIP()), each providing a distinct useful piece of information related to HTTP, so too the (Drupal)RequestInfo object has multiple methods (e.g., getSystemPath() and getRouteName()), each providing a distinct useful piece of information that means something to Drupal, but that isn't defined by HTTP.

some random stuff that we put into the request object which shouldn't necessarily even be in there

If the HTTP request path is /en/about-us, and Drupal handles language prefixes and de-aliases that to /node/1, and then based on the routing component it uses, determines that the "1" corresponds to a node, loads that node, and sticks that node into $request->attributes->set('node'), then why is it inappropriate for it to also stick the _system_path and _route into the attributes too? That's the entire point of what $request->attributes is there for: it's to hold information derived from the HTTP provided info, but that was derived in a framework/application specific way.

static factory methods that return the same class instantiated are usually called create() or createFromSomething()

I think create() or create*() always means that a new instance is created, and that that might matter to the caller. In the case of RequestInfo, the object is stateless other than what's passed to the constructor, so the factory method is free to decide whether to instantiate a new object or not, with no impact on the caller. Therefore, I still think of() would be the best method name.

dawehner’s picture

The main reason why I do prefer get over of is that of is not a verb, which is really uncommon on methods.

Dries’s picture

I'm getting some random stuff that we put into the request object which shouldn't necessarily even be in there.

Having thought about this more, I actually agree that if we could get information like system path, route name, and route managed by an appropriate domain object, rather than an intermediary RequestInfo object, then that would be ideal. For example:

\Drupal::router()->getSystemPath($request)
\Drupal::router()->getMatchedRouteName($request)

What would be the problems with doing that?

dawehner’s picture

What would be the problems with doing that?

One obvious problem would be that people freak out again because they would actually have to type something. Note: in real code you would actually
use dependency injection and never \Drupal.
On top of that we have to accept that the request contains the request derived information, our full stack relies on the request, so
we should actually look it up from the request.

tim.plunkett’s picture

If you mean just move the attribute introspection into the top level service, I see no real gain. Then any method that is passed a request and wants to introspect it must also have had *the entire router* injected so it can ask it things.

If you meant that those methods should *not* introspect the attributes, but instead be able to derive them on their own, that would require us to abandon Symfony CMF's matcher.
The entire architectural design relies on additional context being added to the request's attributes during matching.

dawehner’s picture

The entire architectural design relies on additional context being added to the request's attributes during matching.

Just to be clear, this allows to write a decoupled system.

msonnabaum’s picture

That's not decoupled, that's just obscuring a coupling.

effulgentsia’s picture

Status: Needs review » Postponed

See #2124749-32: [meta] Stop using $request->attributes->get(MAGIC_KEY) as a public API. Postponing this issue until there's agreement (or not) on that. Depending on how that shakes out, this can be reopened as a implementation step towards that, or proceed with some alternative.

znerol’s picture

Re #86

This looks like classic feature-envy to me.

I only answer to this comment because this statement was obviously a reason to abandon the idea here and attempt another start in #2238217: Introduce a RouteMatch class. Please note that the approach discussed here is about the Adapter-pattern, which per definition is about accessing attributes and methods on the adaptee. Talking about feature-envy in this context is nonsense.

effulgentsia’s picture

I just opened a new issue: #2256669: [meta] Finalize module-facing API of getting the matched route name and a matched route argument for a request, and postponed #2238217: Introduce a RouteMatch class on it. Thereby, this issue and that latter one are on an equal footing; we now need to somehow come to consensus on which approach to finish. Please discuss in #2256669: [meta] Finalize module-facing API of getting the matched route name and a matched route argument for a request.

xjm’s picture

Issue tags: -beta target

Untagging in favor of the meta.

catch’s picture

Status: Postponed » Closed (duplicate)