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.
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
Related Issues
N/A
Comment | File | Size | Author |
---|---|---|---|
#90 | request_info-2103301-90.patch | 20.55 KB | dawehner |
#77 | request-info-2103301-77.patch | 20.52 KB | tim.plunkett |
#72 | interdiff.txt | 1.18 KB | dawehner |
#72 | request_info-2103301-72.patch | 20.81 KB | dawehner |
#57 | interdiff.txt | 22.14 KB | dawehner |
Comments
Comment #1
tim.plunkettdebug(drupal_get_route_name())
will become your best friend.Comment #2
tim.plunkett.
Comment #3
dawehnerI 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.
Comment #4
tim.plunkettLike this?
Comment #5
dawehnerHere is an alternative approach.
Comment #6
tim.plunkettI 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::?
Comment #7
tim.plunkettComment #8
dawehnerEven the code looks potentially better I vote to not require to use that object so we keep explicit support for symfony related attributes.
Comment #10
dawehner6: route-name-2103301-6.patch queued for re-testing.
Comment #11
dawehnerJust a note, code that uses the route_name, route object or _system_path is certainly written in a not really decoupled way.
Comment #12
Berdir6: route-name-2103301-6.patch queued for re-testing.
Comment #13
dawehnerRerolled at made the API a bit easier to use by being able to pass in the request as optional argument.
Comment #15
dawehnerUps.
Comment #17
dawehnerThis should fix it.
Comment #18
damiankloip CreditAttribution: damiankloip commentedThese 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.
Comment #19
dawehnerOh wonderful!
Comment #20
dawehner18: 2103301-18.patch queued for re-testing.
Comment #21
jibranThis function should return $this. Don't ask me why ask @tim.plunkett. :)
Comment #22
damiankloip CreditAttribution: damiankloip commentedI'm good with that.
Comment #23
dawehnerNone 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.
Comment #24
sunHm. 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 theRequest
itself?Shouldn't it be passed as argument into individual controller methods instead?
Comment #25
damiankloip CreditAttribution: damiankloip commentedBut RequestInfo just examines the Request that is passed in and returns the value based on that.
Comment #26
dawehnerYeah and the syntax:
- [setRequest, ['@?request=']]
ensures it is always up to date.Comment #27
xjmEdit: I was totally wrong. Never mind.
Comment #28
xjmNow 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.Comment #29
dawehnerOne 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:
and no other work would be needed.
Comment #30
sun#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 theRequest
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.
Comment #31
xjmYeah, 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.
Comment #32
xjmComment #33
damiankloip CreditAttribution: damiankloip commentedOK, 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.
Comment #34
larowlanI think we should also add a method for getting the raw variables - as per attached patch
Comment #35
larowlanStarts moving some core stuff to use getRawVariables() method
Comment #38
Crell CreditAttribution: Crell commentedI 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.)
Comment #39
larowlanPersonally, 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?
Comment #40
znerol CreditAttribution: znerol commented@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.
Comment #41
Crell CreditAttribution: Crell commentedstatic 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.
Comment #42
larowlanI 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.
Comment #43
xtfer CreditAttribution: xtfer commentedStatic 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.
Comment #44
znerol CreditAttribution: znerol commentedLet'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.
Comment #45
effulgentsia CreditAttribution: effulgentsia commentedI think this would be better done as an adapter pattern with a static factory method. Something like this:
And then consuming code would look like:
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.
Comment #46
znerol CreditAttribution: znerol commentedI 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:
Comment #47
dawehnerYeah, we need to accept that we will never allow to switch out the internals of the request object.
Comment #48
effulgentsia CreditAttribution: effulgentsia commented@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.
Comment #50
dawehnerSure. 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.
Agreed. Let's go with the second one.
Comment #51
Crell CreditAttribution: Crell commentedI 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.
Comment #52
larowlanOn 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?
Comment #53
dawehnerReplaced a couple of usecases.
Comment #55
effulgentsia CreditAttribution: effulgentsia commentedThinking 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).
Per above, I think the constructor should take just the $attributes ParameterBag.
I agree with this taking $request. How about renaming the method to
fromRequest()
though?This could then be named
get()
.Do we need the Object suffix? Routes are always objects, right?
Can we add a wrapper in \Drupal, to shorten this to
\Drupal::requestAttributes()->getRouteName()
?Comment #56
znerol CreditAttribution: znerol commentedNo, 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');
withNodeRequestContext::get($request)->getNode();
.Remove this method completely, this has no benefit over
$request->attributes->get()
.Comment #57
dawehnerI 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.
Well, i kinda dislike that internal bit. Most code is dealing with the request object in its full glory already.
Yeah, let's use the same method name as the RequestContext in symfony.
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.
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.
Comment #58
dawehner.
Comment #60
effulgentsia CreditAttribution: effulgentsia commentedBut 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.
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'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.
Comment #61
effulgentsia CreditAttribution: effulgentsia commentedYes 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.
Comment #62
Crell CreditAttribution: Crell commentedFor 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...
Comment #63
dawehner@crell
Do you have an opinoin of getRoute() vs. getRouteObject()?
Comment #64
znerol CreditAttribution: znerol commentedGiven 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:
Comment #65
Crell CreditAttribution: Crell commentedznerol: 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.
Comment #66
tim.plunkettconst 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.
Comment #67
catchLooks like an accidental RTBC?
Comment #68
effulgentsia CreditAttribution: effulgentsia commentedPatch 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:
Not true.
s/routing/Drupal/
Comment #69
catchAlso looks like this could be removed?
Comment #70
znerol CreditAttribution: znerol commentedSure.
Comment #71
effulgentsia CreditAttribution: effulgentsia commentedGreat. In that case, "needs work" for #68 and #69.
Comment #72
dawehnerSure, here are the fixes for these bits.
Comment #73
tim.plunkettMy 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.
Comment #74
Crell CreditAttribution: Crell commentedI'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!
Comment #75
jibranI am already in love with
RequestInfo
class. So +1.Comment #76
ParisLiakos CreditAttribution: ParisLiakos commentedpatch looks great +1 from me too.
I dont want to kick back from rtbc, but just a note
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
Comment #77
tim.plunkettReroll after #1316692: Convert hook_admin_paths() into declarative properties on routes, I didn't address #76. I think this is fine as is...
Comment #78
Dries CreditAttribution: Dries commentedI really like this patch and tried to commit the patch in #72 but it failed. Waiting for #77 to turn green.
Comment #79
sunI 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()
orcreateFromRequest()
(mirroringRequest::createFromGlobals()
) would be more appropriate. The less verbosefromRequest()
,byRequest()
,ofRequest()
would also be fine. The construct should make sense in the scope and context of the calling code.Comment #80
effulgentsia CreditAttribution: effulgentsia commentedI 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.Comment #81
catchNot 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.
Comment #82
effulgentsia CreditAttribution: effulgentsia commentedWould
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.Comment #83
ParisLiakos CreditAttribution: ParisLiakos commentedYes but those methods are not static. static factory methods that return the same class instantiated are usually called
create()
orcreateFromSomething()
Comment #84
znerol CreditAttribution: znerol commentedFully 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):It would also be acceptable to use
adapt
instead ofget
for the factory method. I don't think it is useful to enforce thecreateFromX
convention.Comment #85
dawehnerWell, these create methods thought are basically factories, which we don't really have here, given that we already just pass in everything.
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.
Comment #86
msonnabaum CreditAttribution: msonnabaum commentedWhat 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.
Comment #87
Crell CreditAttribution: Crell commentedI 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.
Comment #88
msonnabaum CreditAttribution: msonnabaum commentedThat's not an argument.
Comment #89
effulgentsia CreditAttribution: effulgentsia commentedRequestInfo 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.
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.
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.Comment #90
dawehnerThe main reason why I do prefer get over of is that of is not a verb, which is really uncommon on methods.
Comment #91
Dries CreditAttribution: Dries commentedHaving 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:
What would be the problems with doing that?
Comment #92
dawehnerOne 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.
Comment #93
tim.plunkettIf 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.
Comment #94
dawehnerJust to be clear, this allows to write a decoupled system.
Comment #95
msonnabaum CreditAttribution: msonnabaum commentedThat's not decoupled, that's just obscuring a coupling.
Comment #96
effulgentsia CreditAttribution: effulgentsia commentedSee #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.
Comment #97
znerol CreditAttribution: znerol commentedRe #86
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.
Comment #98
effulgentsia CreditAttribution: effulgentsia commentedI 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.
Comment #99
xjmUntagging in favor of the meta.
Comment #100
catchClosing as duplicate of #2238217: Introduce a RouteMatch class.