Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem/Motivation
current_path() is basically a wrapper around $request->attributes->get('_system_path')
which is itself considered to be an internal API only
Proposed resolution
Convert the uses of current_path()
to the appropriate alternative.
This involves:
- Adding route support for $batch
- Set the
<none>
route by default before routing got executed. - Add route support for #ajax
- Add
Url::fromRouteMatch()
- Simplify JS apis
- This requires having the route available in tests
Remaining tasks
User interface changes
API changes
Beta phase evaluation
Issue category | Task, before |
---|---|
Issue priority | Critical because its not using the internal _system_path is a critical. |
Comment | File | Size | Author |
---|---|---|---|
#94 | current_path-fixup.patch | 3.38 KB | jbrown |
#93 | 2362227-callback-93.patch | 1.58 KB | olli |
#92 | book-ajax-full-form.png | 48.21 KB | jbrown |
#79 | 2362227-79.patch | 51.84 KB | dawehner |
#79 | interdiff.txt | 1.79 KB | dawehner |
Comments
Comment #1
dawehnerThis is kinda a rough start because we need to add support for routes in quite a lot of places still (if this does not break, the world is kinda broken).
Comment #2
dawehner.
Comment #4
Wim Leers:D
That makes a lot of sense! (Well, route name and parameters, of course.)
This makes sense on the surface, but I don't yet see the full repercussions. (I don't think anybody is around anymore who *thoroughly* knows the AJAX system? If there is, we should ping that person.)
I'm only wondering why
Url
objects are used — is it a conscious choice to not use route name + parameters?Why not just
$settings['url'] instanceof Url
?
And I think you also want to check if
$settings['options']
is set?Missing newline.
It seems this simply doesn't exist anymore?
s/Count/Could/
s/URl/URL/
This needs documentation. It looks as if the Views rendering pipeline *might* have popped the request we added two hunks higher, but not necessarily?
Please explain that here.
Unnecessary change.
Comment #5
dawehnerWe do use url objects in more places now that route_name/parameters.
... You don't review code with storm right?
One hunk above:
No, it does exist, see #2362205: admin/theme/install|update should point to admin/appearance/update|install for more information about that madness.
A bit more explanations here ...
Comment #7
dawehnerMore work, I think we should split this up later though.
Comment #9
Wim LeersNo, why?
Oops, sorry!
Thanks!
#7: could you still post an interdiff, while you probably still have this commit in your git history?
Comment #10
dawehnerUps, sure.
Comment #11
dawehnerRelies on #2362517: Improve default 403/404 exception HTML subscriber: don't duplicate the page render pipeline, use the routing system — add system.403 and system.404 routes to be continued.
Comment #12
catchComment #13
znerol CreditAttribution: znerol commentedYou could achieve the same effect by introducing
URL::createFromRouteMatch()
instead. This would be cleaner because then you do not need to pollute theRouteMatch
class with all sorts of implicit dependencies. See also #2296205: Create a \Drupal\Component\Routing\ namespace and move Drupal-independent routing classes into it (wants to moveRouteMatch
out of core namespace).Comment #16
dawehner@znerol
I like you idea. Will implement that after sending it to the testbot this time. Many of these places though could be replaced by using
the '' route.
Comment #18
dawehnerSome debugging later.
Comment #20
dawehnerThere we go.
Comment #22
skipyT CreditAttribution: skipyT commentedI tried to fix the failed tests for the shortcut module and the view preview.
Comment #24
dawehnerThis could be it.
Comment #26
dawehnerThere we go.
Comment #27
znerol CreditAttribution: znerol commentedGreat that this finally turned green. #13 still applies though.
Comment #28
tim.plunkettWell we don't know if its green yet :)
Comment #29
dawehnerCome on, you should trust me :P
Comment #30
tim.plunkettDrupal\Core\Url is already imported here.
We're calling this twice and don't need to.
Can we get an issue?
elseif
Unused imports
What happens if you specify NULL and then immediately call a method that uses it?
Yay
Comment #31
dawehner@tim.plunkett
This should have been marked as needs work :)
Oh shit, I'm so out of core :)
Well, in that case \Drupal::urlGenerator() is used again.
Comment #32
tim.plunkettThere are still 4 remaining calls to current_path():
Comment #33
dawehnerLet's give it a try and fix all the other instances.
Comment #35
dawehnerMaybe this is it
Comment #37
catchComment #38
jibranComment #39
dawehnerIts green!
Comment #40
znerol CreditAttribution: znerol commentedMissing type hint.
$batch['source_url']
is changed from a plain string to a URL object, right? Seems legit. I have no idea what happens when anUrl
object is serialized, I guess it is okay?Not sure whether this is official policy now, but I guess we should use identity operator whenever possible (
===
).Link to #2371823: Frontpage should maybe store the route name.
This also cropped up in the original issue #2238217-130: Introduce a RouteMatch class. Back then there was no
<none>
route though, so the change is probably ok. But maybe we need to adapt the@return
documentation.Not sure: Do we need to also reset the new
$internalPath
property here?Nice!
Also update the comment.
This looks like a noop.
Use identity operator.
It is
update.theme_update
and the path is/admin/theme/update
(which should be fixed, but probably in another issue).I don't get it why it is necessary to clone the request here. Perhaps add a comment.
Use
assertSame
.I think this should be moved to
UrlTest
unit test.I skipped changes in JavaScript and Views for now.
Comment #41
dawehnerComment #42
znerol CreditAttribution: znerol commentedThere is another issue related to #40.5. It seems to me that this change was necessary in order to make
Url::fromRouteMatch()
work when current route match returns aNullRouteMatch
. This is most probably the case for code running before routing was performed.I believe that in those cases we cannot simply replace
current_path()
withUrl::fromRouteMatch(\Drupal::routeMatch())->getInternalPath()
because that will produce different results.Also
git grep -- '->getRouteObject('
turned up some occurrences where the return value was used in a conditional, i.e. some code is only executed whengetRouteObject
returns something.That said, I think that the changes to
NullRouteMatch
might probably do more harm than any good.Comment #43
dawehnerWell, this is also necessary for tests in which no routing as performed or the installer, where you don't have routing available.
Well, do you have any suggestion how to otherwise solve it? Should we change it to use new Route('') and implement the route
processor in a way that it does not use the route match but rather the _system_path property or figuring out that path
from the
Request
object, manually?Comment #44
znerol CreditAttribution: znerol commentedLet's first try to understand whether this is really a problem. Attached is a patch which bluntly adds some logging to core maintenance mode subscriber (running after routing), user maintenance mode subscriber (running before routing) and kernel pre-handle middleware (running before path alias is resolved).
Add
node/1
with an aliasan-alias-to-node-1
. The results are as follows:Scenario A: only log from core maintenance mode subscriber:
Seems legit.
Scenario B: log before and after routing (i.e. from core and user maintenance mode subscriber):
This is the most interesting test-case, which a) indicates that we really should not attempt to generate Urls from
NullRouteMatch
and b) exposes a static-cache problem inCurrentRouteMatch
.Scenario C: log at all three places:
No surprise here, before resolving the alias
current_path() == request_path()
.Frankly I do not have much of an idea on how to resolve that. I think it is clear from the logs above that we cannot possibly use anything involving the route match before routing actually happened.
The first thing which should be determined is whether it is possible to only use the unaliased-path (i.e. request_path instead of current_path) outside of routed code (controllers). Those occurrences could maybe be substituted with pathinfo?
Comment #45
dawehner@znerol
So would you agree that for these few rare usecases we actually still need something like $request_match->getSystemPath() or something similar on some other object?
Comment #46
znerol CreditAttribution: znerol commented$request_match
?Comment #47
dawehnerEhem
$route_match
Comment #48
znerol CreditAttribution: znerol commentedLet's turn that around: Instead of trying to make route match capable of coping with the installer, make all the crap calling
prepareLegacyRequest()
behave.The interdiff rolls back the modifications to route match and instead pretends that legacy requests are routed. Maybe it is also necessary to adapt
WebTestBase::prepareRequestForGenerator()
, not sure about that.Comment #49
dawehner@znerol
Do you want to file a patch and see whether the test fails?
Comment #50
mpdonadioI like making patches...
Comment #52
znerol CreditAttribution: znerol commentedWhile manually testing I've stumbled upon another issue in both #41 and #50. It seems that the URLs generated in JavaScript are broken. E.g., the toolbar menu ajax callback fails and therefore icons are not displayed in the sidebar. Also subtrees are missing there.
This seems to be related to the changes in the
Drupal.url
function. There is no propertybaseUrl
ondrupalSettings.path
. Is this maybe a leftover from an earlier patch?Comment #53
mpdonadioThis is why the JS broke. _drupal_add_js() doesn't have the equivalent change. Not sure where the / came from in the old version; added it in _drupal_add_js() instead on the JS side. Manual testing `Drupal.url('foo/bar')` seems to work as intended (I am in a subdir locally).
Not seeing .basePath used anywhere.
Comment #54
dawehnerI still think that the NUllRouteMatch should return a NULL route ... just in general it is quite important that code like _drupal_add_js
does not fail when we are not in routing context, as for example exception pages should still work, if possible.
Comment #55
dawehnerComment #56
dawehnerComment #57
mgiffordWould be good to move this issue forward.
Comment #58
dawehnerRerolled for now and fixed the failures. Just to be clear, the statement in #54 is still important. We call code outside of the context of routing, for example on exception pages ...
Comment #60
mpdonadioDidn't into the test fail from #59, that that patch didn't address #52 (likely because I didn't attach the actual path, just the interdiff).
Comment #62
mpdonadioIf this change is what is wanted, then this patch is needed to (1) output drupalSettings.path.baseUrl instead of drupalSettings.path.basePath and (2) update the test to look for baseUrl instead of basePath.
If this change is not wanted, then #58 should be used as the starting point, and the hunk above should be reverted.
Comment #64
mpdonadioThe fail is from a hunk that fell out of the patch for some reason. It is in patches #50 and earlier, but not in #58 and after. I put it back in.
Comment #65
dawehner@tim.plunkett made it clear, that all calls to
current_path()
as well as_current_path()
are removed,so we can remove that functions itself.
Comment #70
mpdonadioThe patch in #1833932-148: Convert theme_system_compact_link() into a #type link is what is causing this to fail now (it just went in a few hours ago); apply that patch in reverse and #64 and #65 pass. It isn't obvious to me why drupal_get_destination() throws an exception in this context b/c of a route match problem.
Comment #72
tim.plunkettBorrowing this hack from _drupal_add_js()
Comment #73
dawehnerSo @tim.plunkett is this patch RTBC?
Comment #74
tim.plunkettI think @znerol is only right academically, and that the original changes to NullRouteMatch are appropriate for the "real world" we live in. Otherwise we're stuck with hacks like the last interdiff.
That said, this is a step in the right direction, let's do it.
Comment #75
znerol CreditAttribution: znerol commentedPrevious patches changed the return value of
NullRouteMatch::getRoute()
fromNULL
to a route instance and that would have led to problems with the static cache inCurrentRouteMatche
as shown in #44. The patch in #72 does not contain changes to any route mach class, that is what I proposed in #48, so I'm obviously happy with that.That might be an API change for JS code. Should probably be reviewed by frontend folks.
Please use the identity operator here.
Once again I want to stress that I am happy with the outcome here.
Comment #76
Wim LeersOnly an internal API change, 99% of JS would use
Drupal.url()
, not any of these individual components.Reviewing the affected front-end parts:
Yep, looks solid.
Better :)
I think dawehner — being a Views maintainer — will have tested this to verify it still works as expected.
Comment #77
znerol CreditAttribution: znerol commentedThe
str.substring()
method takes two integer indexes as parameters. The change here is wrong provided thathref
is a JavaScript string.Comment #78
Wim LeersWell spotted! I guess dawehner didn't test it then.
Comment #79
dawehnerHAHA, no. Now it actually works. Seriously, we need a JS test framework.
Comment #80
Wim Leers:)
Yes, we very, very much need a JS testing framework. Hopefully we'll get it in 8.1 or 8.2.
Comment #81
alexpottAwesome work - very happy to see
_current_path()
being removed. One small thing before this can be committed, the change notice needs to improved to have the JS changes too - a quick scan to make sure we've got everything someone would need to know about.Comment #82
Wim Leers#81: I don't think that's necessary because (from #76):
Nevertheless, people were used to doing things this way in D7 (but got it wrong ~90% of the time…).
I do agree that this is a public API change though:
Updated the CR for both.
Comment #83
alexpottThis issue is a critical task and is allowed per https://www.drupal.org/core/beta-changes. Committed 58480ee and pushed to 8.0.x. Thanks!
Comment #85
anavarreShould this CR be updated too https://www.drupal.org/node/1659562 ?
Comment #86
Wim Leers#85: good point. That CR also still uses
url()
, which no longer exists…Comment #87
jbrown CreditAttribution: jbrown commentedThe change record doesn't explain what to replace current_path() with. It only explains how to get the current URL.
It needs to explain how to update this:
Comment #88
jbrown CreditAttribution: jbrown commentedYou can do this:
but the getInternalPath() method is deprecated. What is the proper way to do this?
Comment #89
BerdirUse the route, not the path, like this:
(Or whatever the route name is ;))
Comment #90
BerdirComment #91
jbrown CreditAttribution: jbrown commented$element['#ajax']['callback'] no longer works - is $element['#ajax']['url'] now the only way? You can't just specify the function name?
Comment #92
jbrown CreditAttribution: jbrown commentedYeah - this commit breaks the $element['#ajax']['callback'] functionality. Attached is a screenshot of the problem from the book module. If I revert this commit the problem goes away.
Comment #93
olli CreditAttribution: olli commentedHere's a fix to the callback option.
Comment #94
jbrown CreditAttribution: jbrown commentedI had to make a few changes to #93 to get it to work.
Comment #95
xjmPlease open a followup issue.
Comment #96
xjmComment #97
jbrown CreditAttribution: jbrown commented#2384545: $element['#ajax']['callback'] is broken, hence breaking e.g. inserting images in CKEditor
Comment #98
xjmThanks @jbrown!
Comment #100
joachim CreditAttribution: joachim commentedThe change record is not quite complete for this issue.
From https://api.drupal.org/api/drupal/includes!path.inc/function/current_pat...
> http://example.com/drupalfolder/node/306 returns "node/306" while base_path() returns "/drupalfolder/".
The CR suggests this as a replacement:
However, that will get you 'drupalfolder/node/306', not 'node/306'.
Looking at the patch, this is the way to do it:
$current_path = \Drupal::routeMatch()->getRouteName() ? Url::fromRouteMatch(\Drupal::routeMatch())->getInternalPath() : '';
Is that really the most succinct way?
Comment #101
dawehner@joachim
The point is that you would almost never have to use the path, but you should rather just pass around the URL everywhere.
Comment #102
joachim CreditAttribution: joachim commentedSure, but in updating modules to D8, the first step will often be to just replace deprecated functions, and change the way things are handled as a second step. So for example in admin_menu, I had:
I can't use URL::__toString() because that gives a subfolder if there is one.
Shall I update the CR with
$current_path = \Drupal::routeMatch()->getRouteName() ? Url::fromRouteMatch(\Drupal::routeMatch())->getInternalPath() : '';
as the 'replacement' for current_path()?Comment #103
BerdirSometimes it's just not possible to do a direct 1:1 conversion.
Drupal 8 actually gives you much better tools for that kind of check, which is actually very limited in 7.x (99% of the ajax requests go either through system/ajax or views/ajax I'd say and that won't match).
What you want in 8.x is the request format. $request->getRequestFormat(). See how MainContentViewSubscriber uses it, 4 common formats are html, drupal_ajax, drupal_dialog and drupal_modal, and you probably just want to do something for html?
I don't think that use case really belongs in a change record, as it is very special...
Comment #104
jrockowitz CreditAttribution: jrockowitz commentedThe most succinct (and reasonable) replacement for current_path() that I have found is
Url::fromRoute('<current>')->getInternalPath()
.Url::getInternalPath() and Entity::getSystemPath() are deprecated but also needed by the path.module's PathWidget to save a URL alias' source as its internal path.
The path.module might be reworked while Drupal 8 is in beta (see #2336597) but it would still be using
Entity::getSystemPath()
with the PathWidget.I am not sure how 'internal paths' could be completely deprecated in D8 core.
Comment #106
alexpottCreated #2426457: Remove request_path as a followup.