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.
Child of #2124749: [meta] Stop using $request->attributes->get(MAGIC_KEY) as a public API. This one is currently used in a number of modules, so marking it as its own beta blocker. We may also want to split out some usages into their own issues. For example, should we refactor SystemHelpBlock and hook_help() to instead have a _help
entry on routes?
Comment | File | Size | Author |
---|---|---|---|
#35 | system_path-2239009-35.patch | 4.82 KB | effulgentsia |
#21 | interdiff.txt | 368 bytes | effulgentsia |
#21 | system_path-2239009-21.patch | 7.78 KB | effulgentsia |
#18 | system_path-2239009-16.patch | 7.74 KB | effulgentsia |
#11 | interdiff.txt | 3.21 KB | dawehner |
Comments
Comment #1
dawehnerRegarding hook_help and routes, we do have an issue already: #2183113: Update hook_help signature to use route_name instead of path
Comment #2
xjmComment #3
dawehnerFrom my understanding system_path belongs onto the the thing we call RequestContext from symfony but maybe simply an extended one from drupal.
Comment #4
effulgentsia CreditAttribution: effulgentsia commentedOpened two small issues: #2259315: Remove _system_path usage from shortcut.module and #2259317: Remove _system_path usage from system_test.module. Combined with #2183113: Update hook_help signature to use route_name instead of path, that gets us to few enough usages of _system_path remaining, that I don't think it needs to be considered a "critical API", and therefore, I don't think needs to block beta. Swapping tags accordingly.
Hm, that's an interesting idea. Request::getPathInfo() probably should stay as the path that was actually submitted by the user (aliased, language-prefixed, etc.), but perhaps RequestContext::getPathInfo() could be used to return the system path. We wouldn't need to extend RequestContext then, unless we wanted to have a separate method for retrieving the user-submitted path, but I don't see why we'd need that method if you could get at that via $request. Anyone else have thoughts on that approach? I guess it would mean that all code that does need system_path (which isn't and shouldn't be much code at all) would need to get injected with a RequestContext object instead of or in addition to $request.
Comment #5
effulgentsia CreditAttribution: effulgentsia commentedTagging WSCCI for the idea in #3/#4.
Comment #6
effulgentsia CreditAttribution: effulgentsia commentedIgnoring the usages fixed by #2183113: Update hook_help signature to use route_name instead of path and #2259315: Remove _system_path usage from shortcut.module, here are all the places (excluding tests):
Calling current_path(), from either procedural code, or from OOP code that doesn't have an injected $request or $request_stack:
Calling current_path(), from OOP code that has an injected $request or $request_stack, so should probably be changed to use it:
Calling
->get('_system_path')
, from OOP code that has an injected $request or $request_stack, so should not be changed to current_path():Comment #7
effulgentsia CreditAttribution: effulgentsia commentedThe reason I broke up #6 by usages of the current_path() wrapper vs. direct access of _system_path, is that if we at some point remove _system_path from $request->attributes and put it on something like RequestContext::getPathInfo() instead (per #3/#4), then so long as we retain the current_path() wrapper, those usages don't break. However, anything accessing _system_path from an injected $request will need to be changed to accessing it from an injected something else (e.g., $request_context).
Comment #8
effulgentsia CreditAttribution: effulgentsia commentedI'm not sure what I think of this yet, but it implements #3 as I understand it. It doesn't convert every usage of _system_path, but hopefully does enough to get the idea. What do you all think?
Comment #9
effulgentsia CreditAttribution: effulgentsia commentedThis, by the way, is the part I'm unsure of. For every current usage of _system_path from a place where we get an injected $request (e.g., a kernel event like this one), we would change 1 line of code to 3, as above. And one that hard-codes the class being instantiated. That seems a bit like a loss. However, it does make conceptual sense I think, in that any code that uses _system_path, what it's really wanting is "what is the canonical path as far as routing is concerned?", which is exactly what the domain of \Drupal\Core\Routing\RequestContext is.
Comment #11
dawehnerThe more I think about this I am not sure whether it is really a good idea.
The only place the request context is used atm. are placed to decouple the routing layer from the request layer completly. It certainly makes sense
to do that, because generate a URL can be used in CLI enviroment, for example.
Because of that pathInfo has a certain semantic meaning we should not change internally. One thing we could do is to provide an additional method, just throw out some idea.
Comment #12
effulgentsia CreditAttribution: effulgentsia commentedDiscussed this with Crell, znerol, pwolanin, and neclimdul at DrupalCon Austin. We concluded that getSystemPath() or some alternate name (e.g., getMatchedPath()?) can be added as a method to RouteMatch in #2238217: Introduce a RouteMatch class. In particular, Crell felt that we should not be using RequestContext within any module code in Drupal. Please reopen this issue if that requires more discussion.
Comment #13
znerol CreditAttribution: znerol commentedUh, I always thought that
_system_path
is routing input, whileRouteMatch
is the output. What about just leaving it on the request for the moment and just focus on removing its usage from module code?Comment #14
Crell CreditAttribution: Crell commentedThe property will likely remain on the request object for the time being, as it needs to be there for routing. The external facing API will be a method on RouteMatch. I agree entirely that no code should be relying on that, which is why we can use that method as a way to document "if you're calling this method you're probably doing it wrong". We do not, sadly, have the time before 8.0 to force-refactor all uses of the incoming path in core nor address all contrib use cases. But this at least gets us a place to communicate to people to stop using it. We can look into removing the method from RouteMatch in D9.
Comment #15
znerol CreditAttribution: znerol commentedWe want to encourage usage of route match but prevent usage of system path. Therefore (and also because of the routing input vs output discrepancy outlined in #13), instead of adding a deprecated method to route match, I propose that we'd rather introduce a new
current_system_path
service (along withDrupal::currentSystemPath()
).Comment #16
effulgentsia CreditAttribution: effulgentsia commentedWe discussed this on that call last week, and @Crell pointed out that it depends on perspective. If you think of "routing" as only what happens in RouterListener, then yes, _system_path is input. However, if you think of "routing" as everything that happens from $kernel->handle() to the controller being invoked, then _system_path is intermediary information that could be considered part of the domain of route matching in a broad sense.
Regardless of above perspective on what constitutes routing, I think splitting out something we intend to pseudo-deprecate away from a brand new API we're creating makes a lot of sense. Here's a patch for review. I'm not sure how to document that it's discouraged: elsewhere, we have
@deprecated in Drupal 8.x, will be removed before Drupal 9.0.
, but does it make sense to state that something is @deprecated without instructions on what to replace it with, and I don't know what to replace the two use cases in the patch with. Any thoughts?I'd rather not add things to Drupal:: that we're trying to discourage people from using. AFAIK, current_path() isn't going away in D8, so I think is fine to leave as the sole wrapper for this for procedural code.
Raising this to beta blocker to match what #2124749: [meta] Stop using $request->attributes->get(MAGIC_KEY) as a public API was prior to me changing that in #2124749-46: [meta] Stop using $request->attributes->get(MAGIC_KEY) as a public API on the assumption that this would be handled as part of #2238217: Introduce a RouteMatch class. However, I don't have a strong opinion on whether this should block beta or not: on the one hand, moving from path-based coding to route-based coding is a fundamental piece of D7 -> D8 module porting, and having a clear answer on how to handle edge case code that must use path could be seen as a key piece of that. On the other hand, if it's only edge case code, then why hold up a beta on it. I think there's a good chance we can get this in before beta in either case though.
Comment #18
effulgentsia CreditAttribution: effulgentsia commentedAnd here's the patch.
Comment #19
effulgentsia CreditAttribution: effulgentsia commentedComment #21
effulgentsia CreditAttribution: effulgentsia commentedComment #22
xjmI believe we discussed also giving it a better name that more clearly explained its purpose/how processed it was, since "system path" doesn't really tell us much (what is the "system"?). We also discussed adding documentation to illustrate exactly what type of thing it is. I.e., to differentiate between:
etc.
Comment #23
Wim LeersI read the entire discussion. It has taken logical steps forward. I think the overall approach is good, and the patch looks squeaky clean (I can't find a single nitpick).
I have only one remark:
Why can't we remove
current_path()
(and_current_path()
) entirely? It's better to have one way to get at something rather than two. Probably I'm missing something, but AFAICT this argues to have two ways to do the same thing.Comment #24
mondrakeSee #2237001: Remove no longer needed _current_path() fallback for early bootstrap
Comment #25
effulgentsia CreditAttribution: effulgentsia commented_current_path()
will go away per #24. So, ignoring that:With HEAD, we have three ways to get the current path:
$request->attributes->get('_system_path')
.$this->requestStack->getCurrentRequest()->attributes->get('_system_path')
.current_path()
.With this patch, that will change to four:
$this->currentSystemPath->get($request)
.$this->currentSystemPath->get()
.current_path()
.\Drupal::service('current_system_path')->get($request)
.The suggestion in #23 is to remove current_path() and change the above four to (first two are the same):
$this->currentSystemPath->get($request)
.$this->currentSystemPath->get()
.\Drupal::currentSystemPath()->get()
.\Drupal::currentSystemPath()->get($request)
.The benefit is that that is more unified. The drawback is currentSystemPath() becomes in autocomplete suggestion whenever you type \Drupal:: into your IDE, but this is a service that ideally no contrib module would ever use, and that over the course of 8.1, 8.2, etc., we'd find a way to remove all the remaining core usages as well. Of course, one could argue that current_path() as a procedural wrapper also has that same drawback: just in the global namespace, which depending on your point of view is either better or worse.
Comment #26
catch@xjm What about 'translated path' or 'resolved path'. I'd also be OK with leaving it as is and trying to document better - this is a concept we should be worrying about less and less.
Comment #27
dawehnerNote: #2284103: Remove the request from the container will remove the request from the container, so we maybe should consider supporting getting the system path directly from the stack.
Comment #28
Wim LeersI don't care too much about the namespace in which this non-DIC version to get the system path lives, I only care about not having two code paths for the same thing. That's the sole reason I argued in #23 to remove
current_path()
in favor of always using the newCurrentSystemPath
service.If we want to make it explicit that we don't condone using that service, then why don't we just not create
\Drupal::currentSystemPath()
, hence forcing people to use\Drupal::service('current_system_path')
?Comment #29
effulgentsia CreditAttribution: effulgentsia commentedYeah, along these lines, I took another look at the list at the bottom of #6, and think that we can just remove all but 2 or 3 non-internal usages, and instead of needing a service to support those 2 or 3, can just add a comment for them explaining that we're accessing something we shouldn't be in order to maintain a legacy feature?
Specifically, the two that I suspect we'll be stuck with are:
- Drupal\system\Plugin\Condition\RequestPath (not in the list in #6 due to being added after that)
- Drupal\views\Plugin\views\argument_default\Raw
I think both of those are flawed concepts (we should be implementing block visibility conditions and Views arguments from route names and named route parameters, or from breadcrumb or menu information, not from the system path), but I don't know that we'll be able to make that change in D8.
I think user module's MaintenanceModeSubscriber is an interesting use of _system_path given that it needs to happen pre-routing. I wonder if it could be implemented with a maintenance mode router though, that only contains the 5 or so hard-coded routes. Since that might be problematic, that's why I wrote "or 3" above.
As to the rest of the list, I think CsrfAccessCheck, FormSubmitter, and MenuTree can be refactored to use route name and params, ExceptionController can be refactored to use drupal_get_destination() (or a service equivalent if that's ever made), and PathListenerBase, RouteProvider, and UrlMatcher can be considered internal consumers of _system_path, so can continue accessing it from $request directly without problems.
Not sure where that leaves this issue then. Perhaps all that's needed before beta is to see if we agree on the above plan and file (but not necessarily complete) the corresponding conversion/documentation issues?
Comment #30
Crell CreditAttribution: Crell commentedMaintenanceModeSubscriber doesn't use _system_path. It uses its own _maintenance_mode flag. Looking that subscriber over, 1) It's grown considerably since I last looked at it; 2) I'm pretty sure if we move isSiteInMaintenance() off to a stand-alone service we could eliminate that property entirely (and do other cleanup there that seems useful to do, unrelated to this issue).
In general though, I agree that it seems we're slowly refactoring this problem out of existence anyway and should continue to do so. If we must leave a few cases around, per #29, we should mark them deprecated even if there's no replacement yet; they may live for all of D8 but we can at least signal "stop doing that".
Comment #31
znerol CreditAttribution: znerol commentedRegarding maintenance mode, see #2288665: Remove _maintenance request attribute and replace it with a maintenance mode service and #2288911: Use route name instead of system path in user maintenance mode subscriber.
Comment #32
Wim LeersIt's also being removed from
MenuTree
.Comment #33
vijaycs85Comment #34
effulgentsia CreditAttribution: effulgentsia commentedI'm going to open non-beta-blocking issues for #29, and then upload a patch here that just has @todos pointing to them. That's a completely different approach than #21, so setting to Active until I upload that.
Comment #35
effulgentsia CreditAttribution: effulgentsia commentedAlrighty, how's this for a semi-cheating way of resolving this beta blocker?
Comment #36
catchAdding the @todos works for me, looks like the follow-ups will involve changing service definitions so I think we should make those major/beta target (i.e. will require container rebuild on update at a minimum).
Comment #37
xjmI'm okay with that too. Edit: I've bumped the priority of all the children.
Comment #38
Dries CreditAttribution: Dries commentedCommitted to 8.x. Thank you.
Comment #40
xjmThere are a few change records that have examples using it that need to be updated:
https://www.drupal.org/list-changes/published/drupal?keywords_descriptio...
One even has a reference to this issue. :)
Comment #42
znerol CreditAttribution: znerol commented