Problem/Motivation
This issue is blocking critical #2368653: Replace _l in all places (3) besides one.
While in general, it's agreed that links in code should always refer to routes, there are nevertheless certain valid use cases for linking to paths instead:
- As a developer, I want to handle user-entered paths, such as those coming from Menu UI, Link, or Shortcut modules.
- As a developer, I want to write automated tests that uses paths to simulate a user entering paths into a browser.
- As a developer, I want to create links to un-routed paths located within my Drupal site, such as
README.txtormisc/druplicon.png. - As a developer, I want to create links to un-routed paths located outside my Drupal site, such as
/wordpress/admin.php. - As a developer writing a custom module for a single site, I know the canonical path of a resource I want to link to (e.g., on my site, I know that it's /user/register and not altered by any other module), and want to write code that links to it rather than spending my time figuring out the route name of that.
- As a developer of a contrib module, I don't accept the premise that it's legitimate to alter the paths of Drupal routes, because I think that Drupal's other tools like URL aliases and hook_url_(in|out)bound_alter() are better since they don't require changes to the routing info. Therefore, I want to write my contrib module's code to link to resources based on their default routing path, ignore the integration failures with modules that alter them, and not have Drupal core tell me that I can't do that.
It's important for core to set the direction here on the proper way to accomplish these use cases, because otherwise developers are likely to find and share the Url::fromUri('base://...') workaround, thus largely negating one of the main benefits of the new routing system (the ability to link to a "machine mame" which is less likely to change than a path).
Proposed resolution
- Add support to
Url::fromUri()for a user-path:// scheme and document the difference compared to base:// - Add logic to to fallback to behavior of base:// if not route is found. This doesn't change any API but improves the DX a lot.
- Add a giant docblock why and when to use it, and why this function is not preferred. This will also give us the possibility to group all the needed info for the developers into one place.
Remaining tasks
Pick a suitable name, create a patch, add the docs.
User interface changes
None
API changes
API addition
Postponed until
#2412509: Change LinkItem.uri to type 'uri' rather than 'string' and introduce user-path: scheme
Original report by @aspilicious
Problem/Motivation
Lots of people raised concerns that there wasn't a function to generate url's based on paths.
The argument always was that almost everything could be done with routes.
#2364157: Replace most existing _url calls with Url objects proves again that, this is a lie. More than 40
$url = Url::fromUri('base://somepath')
instances are introduced.
If even core can't keep it's promise we shouldn't force such crappy DX on every drupal developer around the world.
If we don't improve the DX, there will be a lot of questions on drupal.stackexchange and eventually several blogposts explaining the fromUri('base://'..) hack.
In the end we will have the following
1) Nobody gets why it needs to be so hacky (except for the small group of people worked on it)
2) It uglifies the code
3) Everyone will use it anyway once the hack is known.
| Comment | File | Size | Author |
|---|---|---|---|
| #88 | user-path-url-2405551.88.patch | 13.74 KB | larowlan |
| #88 | interdiff.txt | 1.58 KB | larowlan |
| #87 | increment.txt | 1.07 KB | pwolanin |
| #87 | 2405551-87.patch | 13.75 KB | pwolanin |
| #84 | increment.txt | 11.82 KB | pwolanin |
Comments
Comment #1
dawehnerIt must be clear that this will not apply url aliases, unless enforced, language prefixes and what not.
The documentation has to say that this is not the API you want to use.
Comment #2
larowlanShould it be deprecated too?
Comment #3
dawehnerIdeally things like that should be something like not recommended to use, but semantically its different than @deprecated
It would be great to have a generic categorisation for those kind of APIs.
Comment #4
aspilicious commentedThis
is the reason why we should have this function, just to explain this.
Comment #5
dawehnerWhat do you think about this piece of documentation?
Comment #6
aspilicious commentedThe wording could be improved, but the message sounds ok to me. :)
Comment #7
dawehnerLet's post a patch to attract people
Comment #8
pwolanin commentedWe should have a 2 step API - resolve a path (user input) to a route, and then render the route.
I don't think a single method encourages the right behavior, and using base:// is supposed to be annoying or a flag that you are doing something out of the ordinary.
Comment #9
dawehnerI though would like us to add that method, given that there is no real place that we document why ::fromPath (or the replacement) is entirely probably
not what you want to do. People look at code in core ...
Comment #10
effulgentsia commentedThere's ambiguity in the word "path". Suppose you're running a site without clean URLs and in a subdirectory called "d8", so when you're on the node 1 page, you're on:
http://example.com/d8/index.php/node/1Say from here you want to a) output a link to the
user/login"path", or b) output a link to thecore/misc/druplicon.png"path".- a) would need to be output as
/d8/index.php/user/login- b) would need to be output as
/d8/core/misc/druplicon.pngIn other words, a's path needs to be interpreted as "relative to Drupal's front controller" and b's path needs to be interpreted as "relative to the location where Drupal is installed".
In discussions related to #2351379: [meta] Define, then support exact use cases for link generation and storage, we seem to be settling on the need that when paths are entered in the UI (e.g., a link field), we need logic that can intelligently figure out which kind of path it is (e.g., run inbound path processors, then check if it can be routed, if yes then a, if no then b).
However, I'm not sure if such logic belongs anywhere in the Url class (maybe that should be a service that is only used by code that deals with user-entered text?).
I'm concerned that this issue's approach of adding a Url::fromPath() method that does not implement the above logic, but only supports b and not a, would add more confusion than it solves (despite the nice docs).
Comment #11
effulgentsia commentedTo clarify that concern more, #7 would introduce the idea that within the Url class, "path" (in the sense of "fromPath()") means only #10b. Whereas in other issues, we'll introduce the idea that when dealing with user-entered "paths", then "path" could mean either #10a or #10b. That's an inconsistency I don't like.
Comment #12
dawehnerThat is a fair argument. Maybe we could copy some sort of documentiation onto ::fromUri?
Comment #13
effulgentsia commentedPart of the problem is that many, but not all, of those core usages are incorrect. For example,
Url::fromUri('base://core/authorize.php');is correct, since base:// is for #10b.is incorrect, since 'destination' is an example of #10a. If we really need support for generating URLs from these kinds of paths, should we add another scheme (e.g.,
front://)?+1. Perhaps after answering the above question on whether to add a front:// scheme?
Comment #14
pwolanin commentedWell, this looks wrong to me since we could process the destination path into a route and then render an absolute URL?
Comment #15
mpdonadioAt one point in #2364157: Replace most existing _url calls with Url objects, I tried to introduce the attached (didn't try to copy the doc suggestions above).
I also think we should open an issue (I'll do it) to audit core for uses of base:// and convert to route or document why base:// is needed.
Comment #16
webchickNot exactly sure what tag to put here, but basically wanted to note that on the Mega Menus Meeting™ that we had earlier today which came up with #2407505: [meta] Finalize the menu links (and other user-entered paths) system, this issue had some controversy around it. The DX improvement is obvious, but because of the DX improvement, people were worried that we could end up with more people relying on this function than the more proper Url::fromRoute() and thus break one of the primary use cases of the new routing system (that you should be able to move paths around without screwing site builders/other peoples' code).
So carry on, but just wanted to note that unlike other issues linked off of the "Finalize menu links" issue, this one is not necessarily approved.
Comment #17
amateescu commented#13 sounds to me like we want
routed://andunrouted://schemas. At least those should be more self-descriptive thanbase://andfront://, but maybe it's just me :)Comment #18
pwolanin commentedSO, I still think we should make this 2 steps - I don't think the DX is improved by obscuring the fact that the code is calling into a relatively slow process that shoudl be used only as an edge case or on certain user input.
Comment #19
pwolanin commentedIn other words - I think any approach using base:// for something that maps to a route needs work
Comment #20
dawehner@pwolanin
Well, but note: this is not the point of this issue though. Its just about making the base:// part easier. I agree that fromPath is a bad name.
Comment #21
dawehnerAlright, let's be clear, what we deal with and shift the issue.
Comment #22
wim leersI'd prefer
*is*overIS, if only because api.d.o uses a font that makes it super difficult to distinguish between an uppercaseiand a lowercasel.What about:
s/Drupal controlled/Drupal-controlled/
This route can be altered to improve UX; e.g. the controller can be replaced to provide a content creation UI.
I didn't know about this one :)
Comment #23
wim leersI reviewed this, but I share this concern:
Comment #24
mpdonadioCan we mark this as an @internal?
Comment #25
wim leers#24: ooh!
Comment #26
effulgentsia commentedInstead of an
<invalid>route, should we throw an InvalidArgumentException?Comment #27
effulgentsia commentedComment #28
dawehner@effulgentsia
We talked about that on IRC and went with
<invalid>because its potentially userinput we deal with, so we don't want every caller to have to deal with it manually.Instead with the introduction of
<invalid>, we have a single place which people could swap out to improve broken links, by pointing to a different path.Comment #29
dawehnerThank you wim for your review!
Ha
Still think its a hack, but this is how we made
<none>to actually work as expected.Comment #30
dawehnerI decided to not put
@internalgiven that there are a limited set of usecases, also for contrib.Comment #31
aspilicious commentedI'll have to buy beer (or other drinks) for everyone in this issue :)
Comment #32
webchickJust a thought. If the main use case is user-entered paths, can we call it
fromUserEnteredPath()? That makes it a bit more of a mouthful to scare people away.Comment #33
amateescu commentedBuilding a bit on the schema suggestions from #17, how about
fromRoutedPath()and maybe a siblingfromUnroutedPath()?Edit: my keyboard ate the 'd' in
fromUnroutedPath()above :)Comment #34
dawehnerI really like this idea!
On top of that I also replaced some of the existing bad usages of
base://Comment #36
dawehnerAlright, you can't handle random paths with that.
Comment #37
pwolanin commentedI like webchick's suggestion of indicating a user input path in the method name.
Comment #38
effulgentsia commented+1 to #32 / #37. What do you think of this as the implementation and docs of that?
Comment #39
pwolanin commentedWe should never use base:// here. If it can't be routed, it's a broken link
However, I do think if it's an external URL (or just http/https?) handling that case is useful for cases like link field.
Comment #40
pwolanin commentedHere's sort of a merger of the approaches.
Comment #41
David_Rothstein commentedAs a human who sometimes writes computer programs I like Url::fromUserEnteredPath() also... it's easy to understand.
Is this actually a good example? Altering routes in this manner is very fragile (because of the broken links problem), and there are better methods that aren't fragile, aren't there? For example, to do this in Drupal 7 I would use hook_url_inbound_alter() + hook_url_outbound_alter(), which accomplishes the same goal without the risk of breaking any existing links. So I'd expect Drupal 8 developers to do the equivalent, and I'm not sure why it's necessary to support a less robust alteration method even if it's technically possible.
Because of that, do you really need to discourage people from calling this method, and would be so bad to just name it fromPath() after all?
But if it's worth keeping at fromUserEnteredPath() and discouraging them anyway, I'd suggest the documentation just focus more on the general idea that route names are permanent while paths can be changed in various ways, so it's better for code to use the route name when it knows it.
Comment #42
wim leers+1
Comment #43
pwolanin commentedok, updating title
Also, @David_Rothstein yes - the code comment probably need some more work as you suggest.
Comment #44
effulgentsia commentedWhat if the user enters
core/LICENSE.TXTorpath/to/silex/page? You sure #38's fallback tobase://isn't what we want? See also #10.Comment #45
dawehnerThat is valid, there might be usecases for it.
Comment #46
mpdonadioLinking out to README and INSTALL files has become somewhat common practice in hook_help() in contrib modules.
I also work with a decent number of clients who have things parallel to Drupal in their DOCROOT, usually something legacy that they didn't want to migrate into a new Drupal site and didn't want URLs to break.
Comment #47
pwolanin commentedIn hook_help() or other code they can use base:// with the fromUri method - again this is only about handling user input.
Also - though I considered an explicit check and removal - base:// in the actual user input would work, and if that scheme is not friendly enough, we could change it, but I think this use case is rare enough you should have to do something that obviously makes clear you are linking to something not in Drupal.
Comment #48
dawehnerWell, it should be clear that users don't need to be developers, so for many people it won't be obvious what
base://would mean.On the other hand the question is what kind of people need to use
base://Personally I kinda prefer to not require
base://kinda, but this would also mean that we can't validate whether a path is valid in Menu anymore.Comment #49
pwolanin commentedSo, yes, if the user manually enters that scheme, we won't try to validate, alias, or otherwise process the input other than adding the base path. Given that this should be a rare use case, I think explicit user intent is better than trying to guess or fall back.
Comment #50
mpdonadioOK, so I have read this several times, and thought about use cases with sites I have built for clients. Some of this is just repeating what others have said.
We have a patch in #40 that will take
This should cover all of the use cases for menus and link fields.
As for whether we should use a version with base:// fallback, I think there are two reasons not to do it.
1. It is an edge case that will impact few users. And, the base:// part could be handled via interface design and/or contextual help.
2. It will prevent base:// misuse. The fromUri() uses are easy to track down and notice. Burying this inside the method will make it easier to abuse and harder to remedy.
I kinda hate the method name. I can't quite explain why, but it bothers me. I don't have an alternate. It would be nice if PHPDoc supported something like @dontuse.
I think the docs are fine.
I think this is RTBC.
Comment #51
effulgentsia commentedHere's some reasons to do it:
/node/add/article?destination=README.txtand after submitting the form, get redirected to/README.txt.README.txtas the URL of a link field, and it will successfully link to that.$url->isRouted(). There's even docs in that patch about that.blogbeing able to survive a change of that page's implementation from a Drupal View to a Drupal Panel Page without needing to edit which entity that link points to (the value of which I'm kind of "meh" on, since editing a link to point to a new entity is a tiny amount of work compared to building the panel page), then why not also allow it to change implementation from a Drupal View / Panel Page to a Wordpress app that sits alongside Drupal? I actually care about that use-case more, since it can't be implemented as an entity reference, so for me is a much more legitimate use of paths than the View/Panel polymorphism.base://some_pathif they want to link to a non-Drupal path vs.some_pathif they want to link to a Drupal path makes no sense. Why should a content editor editing the value of a link field need to know which application on their site is implementing a particular URL? Say I want to link to/shop. Maybe that's implemented with Drupal Commerce; maybe it's implemented with a separate non-Drupal commerce app; maybe my content is on a D8 site, but the commerce portion of my site is still on D7, cause I haven't been able to port it to D8 yet. As a content editor, I don't care; I just want to link to it.Comment #52
effulgentsia commentedAnd a delayed response to #41:
I think it's important to distinguish between:
For the former case,
hook_url_(in|out)bound_alter()is usually the better approach, so that the site's old links keep working.For the latter case, I don't think
hook_url_(in|out)bound_alter()is optimal, either conceptually, functionally, or performance-wise. We're used to it from Drupal 7, because a Drupal 7 site is required to contend with Drupal 7's baggage of code referring to pages by their Drupal path, since there's no other identifier to pass tourl(). But with Drupal 8, we can get rid of that baggage, because for both entity and non-entity resources, we have separate, stable identifiers available to us; we just need to use them.Comment #53
pwolanin commented@effulgentsia - well, we won't be using this method to handle "destination" query strings since it would lead to open redirect vulns, or we'd have to be wrapping it with extra checks.
For link fields, Views, menu links, etc - the patch is establishing a pattern to copy, I don't think this exact code will be called - this is more of a contrib helper I expect.
I tend to agree with mpdonadio in terms of the arguments against, especially on the DX front and this being an edge case to link to non-Drupal content.
I'll also note that the idea of base:// was from the HTML base tag https://developer.mozilla.org/en-US/docs/Web/HTML/Element/base
Comment #54
dawehnerWell, I think its fair that we want for all those examples to also allow to link to external URLs. Having a central place for this kind of code seems sane for me.
Comment #55
wim leersThis has been thoroughly thought through. This patch presents a good step forward. I've stepped through it and can't find anything to complain about.
In the reroll, a few nitpicks have been fixed.
Comment #56
effulgentsia commentedPer #51, I'm happy with that, but AFAIK, @pwolanin isn't, so setting back to "needs review" to give him a chance to comment. If that's the only sticking point, I'd be ok with either version being committed here, and a follow-up opened to continue that debate, but I want to make sure that anything committed here is acceptable by Peter at least as an interim step.
Per #53, do we need to be careful about that (i.e., ensure that $url isn't external)? I'm not actually all that clear on when open redirects are ok and when they aren't (e.g., in this case, it's added to a link rather than a redirect, but not sure if that matters).
Comment #57
David_Rothstein commentedThat would be a vulnerability, yes. A relatively minor one, but it could be used in a phishing attack... No one would really suspect that clicking a system-generated "cancel" link on a form could take you to an untrusted website.
Comment #58
David_Rothstein commentedThat's a good point. However, I feel like we're already talking about an edge case here (most people who want to do this kind of thing, if they even want to do it at all, are going to use URL aliases rather than writing code). And now we're talking about a subset of an edge case :)
And on top of that we aren't even talking about making the edge case possible, just about improving the developer experience (and maybe the performance) of that edge case.
But the tradeoff is that we hurt the developer experience for something that is pretty much a 100% use case (linking to things). If I want to link to the 'node/add' page in my code, calling
Url::fromPath('node/add')is pretty easy to understand. CallingUrl::fromRoute('node.add_page')is considerably more difficult. First of all conceptually, but even after you understand the concept, it's still more difficult because there is no pattern that allows you to figure out the route name. So you have to drop what you're doing and go look it up somewhere else, and find out that it's 'node.add_page' after all (and not, say, 'node.add' or 'entity.node.add' or a million other things I might have guessed).I don't think that's a good tradeoff. I think this issue should give serious consideration to the original suggestion of
Url::fromPath()and let the chips fall where they may.Comment #59
David_Rothstein commentedThis could be fixed, I suppose, although I suspect it may be too late for Drupal 8. I filed an issue: #2412709: Route names should have some kind of logical relationship to their canonical paths
Comment #60
wim leersUnfortunately, the fact is that Drupal 8 has this routing system and thus code must generate links by using routes. Since, we can no longer change the Drupal 8 routing system, we'll have to do things that way.
Don't get me wrong, I have similar reservations…
This issue was AFAIK solely about having a simple API to disambiguate user input, #58 broadens the scope. For which I fear it is too late.
Comment #61
David_Rothstein commentedThis issue was originally about the following (https://www.drupal.org/node/2405551/revisions/8004803/view):
So I'm not trying to broaden the scope here, really more like making sure it stays on track with the original goal...
Basically, we can definitely add documentation that mentions (and maybe even encourages) the alternatives, but unless we're totally convinced that user-entered paths are the only legitimate reason a developer might choose to call this function, "fromUserEnteredPath" might not be the best name after all.
Comment #62
wim leersFair; that indeed was the original scope. I thought it got downscoped, but perhaps I'm wrong.
I just fear we're going to enter an endless discussion if we're going back to the original scope, because introducing
Url::fromPath()and considering it equally fine to use asUrl::fromRoute()is almost equivalent to saying we think it's fine to use the routing system as little as possible, sticking to D7-thinking.Now I'll shut up and let the routing system experts speak their mind :).
Comment #63
berdirI guess one question is what we consider as "user" ;)
Take simpletest for example, it is absolutely common to use system paths there, and changing that would be crazy. Arguably, we are entering paths there like a user is entering paths into the browser, so is that a user too?
There is quite a bit of back and forth on that in #2364157: Replace most existing _url calls with Url objects, and trying diferent approaches in drupalGet()/Post(). It supports system paths or Url objects now, but it is still using generateFromPath() for the system paths, base:// did not work, for example because 'base://' (frontpage) is not supported.
Comment #64
pwolanin commentedI think handling user input is the valid use case - let's stick with "fromUserEnteredPath"
Comment #65
effulgentsia commentedWhat about
fromSiteSpecificPath()orfromSitePath(). The idea being that at least in theory, if not in practice (see #58 about arguments that this is an edge case), a site should be in control of its URLs/paths, not be forced into paths that Drupal hard-codes. So, any caller of fromSitePath() would be making an assumption that is in-theory site-specific. Examples of acceptable uses of that:Comment #66
webchickOk, if we're going to go into another bikeshedding round, then we need to lay out the valid use cases for this new method. I made an attempt at doing that in the issue summary.
Comment #67
effulgentsia commentedAdded 2 more use cases based on #58 / #65. Wording might be a bit too confrontational, so please edit as needed.
Comment #68
webchickTurning that into a numbered list so we can discuss them.
Comment #69
webchickMy personal feeling is:
Use case #1 is definitely worth accounting for (it's basically why we're here), and #2 is basically a variation of #1. Between the two of those, we hit basically all of the reasons we currently use base://... in core.
Use case #3 and #4 don't necessarily need to be accounted for with this new method. They come up rarely, so the cartwheels involved in
Url::fromUrl('base://xxx')are fine.While I can see them being valid use cases, in that there are people out there who feel that way (including myself at times ;)), I actually don't actually think core should care about catering to #5 and #6. Or at least, I don't see a way we can cater to them while avoiding presenting a schizophrenic and inconsistent DX for our routing system that negates the main reason for doing it (so that you could link to machine names rather than paths for better reliability).
Also, individual developers whose feelings are captured in #5 and #6 can simply choose to ignore the documentation that tells them #1 and #2 are the only valid use cases and use them that way if they want, and others can file bug reports if it screws up their sites (similar to how you can file a bug report if a module forgets to use t()).
So if it's true that #1 and #2 are the only use cases we want to promote as a best practice, I feel that the method name should be centered around those valid use cases, not centered around something more generic that tries to capture all six use cases and muddies the water for the primary two.
Comment #70
David_Rothstein commentedI like where you're going with that concept, but I had to read the whole paragraph to understand the name :) I don't have a better suggestion for a name that communicates that idea though.
I think it would be great to mention that "site-specific" concept in the documentation (because it's a good way to understand it) but I think the method could still just be called fromPath() in that case.
What this really reminds me of is db_query(). It is recommended to only use it for SELECT queries, and it's documented as such. But the word "select" is not in the function name; that would just seem heavy-handed, wouldn't it? In my experience it works well. Most code I see uses it in the recommended way. Every once in a while I see it used on a non-SELECT query, usually in custom code on a MySQL site where it doesn't matter anyway. And if it's ever used that way in a contrib module and someone can't use the module with Postgres (or some other database) because of that, yup, they file a bug report... Overall, the market decides.
Comment #71
webchickAlso, if #70 is the generally accepted feeling, then we should just go back to Url::fromPath() and be done.
Comment #72
joelpittet+1 for #70 Url::fromPath()
Comment #73
effulgentsia commentedI like fromPath(). I think #70's analogy to db_query() is a good one.
If we think we'd still like some middle word in there to disambiguate "path", here's some more ideas to consider:
fromRelativePath(): "relative" conveys the concept that there's some logic in the code to figure out what it's relative to. Which is actually quite tricky. Suppose you're on a multilingual site without clean URLs and you're on the pagehttp://example.com/drupal-subfolder/index.php/fr/node/addand this new method we're adding gets passed "user/register". There's a lot of possibilities for what that is relative to (how many parts of the current URL do you strip off before adding the passed-in path?). Turns out, the logic we want is for it to be relative tohttp://example.com/drupal-subfolder/index.php/fr, but there's nothing about the concepts of URLs and paths for you to know that: you need to know some stuff about how Drupal works to know that's what is wanted.fromInboundPath(): we already use the term "inbound" when referring to inbound path processors and hook_url_inbound_alter(). The concept is that the path that is being passed in is the one that if it were typed into the browser address bar (so long as it was also prefixed by the appropriate parts per above) would yield some resource, and that is the resource we want to link to.Not sure if either of those suggestions is helpful. Just throwing them out there in case it resonates with anyone.
Comment #74
joelpittetAlso like @effulgentsia's suggestion of
Url::fromRelativePath(). +1 to that or still cool withUrl::fromPath()mostly because it's become expected that if you enter a path in D7 land it will language and site base path prefix for you.Don't care for
Url::fromInboundPath(), thanks for the rationals @effulgentsia.Comment #75
effulgentsia commentedOk, so if we're choosing between fromPath() and fromRelativePath(), here's some more thoughts. Per #73.1, suppose you're currently on
http://example.com/drupal-subfolder/index.php/fr/node/add. IMO, the behavior of the function should include the following:user/register, the resulting URL should behttp://example.com/drupal-subfolder/index.php/fr/user/registeren/user/register, the resulting URL should behttp://example.com/drupal-subfolder/index.php/en/user/registercustom-script.php, the resulting URL should behttp://example.com/drupal-subfolder/custom-script.php../custom-script.php, the resulting URL should behttp://example.com/custom-script.phpSo, arguments for
fromRelativePath():Arguments for
fromPath():Per #57, regardless of name chosen, we should probably remove support for passing in full URIs to this function; the caller can call fromURI() for that explicitly if they want to (UIs that want to handle both relative paths and full URIs can include the appropriate if statement in order to call the right function), so IMO, that's not an argument against either name.
Comment #76
pwolanin commentedI think we need a set of schemes and at the API level only support fromUri().
The UI/widget may add a scheme.
Comment #77
aspilicious commentedJust to be sure, I also need Url::fromPath() at the API level that's why I opened the issue in the first place...
So please tell me I'm just misunderstanding your comment.
So *ïf* I understand your comment you're basically just ignoring everything I'm saying in the summary...
And than my code should be:
Correct? (I hope not)
Comment #78
aspilicious commentedTalked with pwolanin
That makes sense to me and is ok for me. And removes my concerns :)
Comment #79
catchNot sure tests is really a valid use-case. If we accept the idea that you should be able to update the path for a route and not have to update any other code, then having to update tests breaks that. If we're saying it's fine to have to update test code, then that seems valid for all other module code as well.
Comment #80
wim leers#2412509 is blocked on exactly this discussion:
Depending on what we decide here regarding "paths", and how they should be stored (i.e. whether
/node/1is stored asnode/1orpath://node/1), we need to pick a different route (pun intended) in that other issue. See #2412509-4: Change LinkItem.uri to type 'uri' rather than 'string' and introduce user-path: scheme for details.Comment #81
effulgentsia commentedOk, so, meanwhile, the direction in #2407505: [meta] Finalize the menu links (and other user-entered paths) system is now for all of core's "user enters a path and we need to make a link to that" UIs to use the link field. #2235457: Use link field for shortcut entity has already landed and #2406749: Use a link field for custom menu link is being worked on.
Per #78, pwolanin and I agreed that #2412509: Change LinkItem.uri to type 'uri' rather than 'string' and introduce user-path: scheme is desirable: in other words, that to make the link field's storage model most proper, we need some new scheme name to apply to the paths that users enter into that field. During that discussion,
pathwas the best scheme name we came up with, but I'm now recommendingundeterminedas that name. See the summary of #2412509: Change LinkItem.uri to type 'uri' rather than 'string' and introduce user-path: scheme for details. Still open to better naming ideas; if you have one, please comment in that issue.Assuming that happens, then other code that wants to generate a link to some user-entered-path that isn't the value of a link field (e.g., the token example from #77 or the destination query parameter example from #56.2) will be able to call
Url::fromUri('undetermined://' . $path). Depending on what people think of #79 and #58, we may also want a scheme for "we know it's routed", such asrouted://proposed in #17. I think it's still debatable on whether we want that though, or whether code should use 'undetermined' even when it knows the path to correspond to a Drupal route. We can always open a dedicated issue for discussing that if people care about it.Which will then leave us with the question of do we still want this issue to add a Url::fromPath() method that would be a very simple wrapper, as so:
@pwolanin argued that we should not have that, since why add a minor convenience wrapper to something we would prefer people do as rarely as possible. That reasoning makes sense to me. Others might disagree though and prefer to have the convenience wrapper.
I think it makes sense to postpone this on #2412509: Change LinkItem.uri to type 'uri' rather than 'string' and introduce user-path: scheme, so doing so. Please unpostpone if you disagree.
Comment #82
pwolanin commentedre-roll #55 for conflict in core/tests/Drupal/Tests/Core/UrlTest.php
I don't think "undetermined://" is scheme that tells me much. path:// would be a little more nature?
do we need to postpone? other than picking the scheme name, it seems like they are independent.
I'll try twitching to the path:// schema next.
Comment #83
pwolanin commentedFrom the other issue - possibly settled on a pass trying to implement as user-path://
Comment #84
pwolanin commentedOk, here's a quick pass at the approach of using a URI.
Comment #86
catchIf we want to support both Drupal and non-Drupal paths (and Drupal paths that might not resolve yet), then this sounds good.
path or user-path are OK with me.
Don't like 'undetermined' much I misread it as 'undefined' first glance, then thought of JavaScript.
Then when I read it again and saw 'undermined' properly I thought of John Cage and indeterminacy.
Comment #87
pwolanin commentedoops, forgot to make the route parameters as arrays in the test.
Comment #88
larowlannitpick > 80 :(
one to many any
Over in #2412509: Change LinkItem.uri to type 'uri' rather than 'string' and introduce user-path: scheme I proposed UrlHelper:userPathFromUri() (we will have 5+ instances of this code) depending on which goes in first (assuming that it gets the nod) we should use that here too.
Patch fixes the nits, this is rtbc in my book, item 3 can be a follow-up if/once #2412509: Change LinkItem.uri to type 'uri' rather than 'string' and introduce user-path: scheme is in
Comment #89
wim leersOh, wow, this also introduces the
user-path://scheme, just like #2412509 does. But catch just un-RTBC'd that issue (see #2412509-33: Change LinkItem.uri to type 'uri' rather than 'string' and introduce user-path: scheme) because we should first decide whether to useuser-path://(currently used in both that and this patch) oruser-path:, which is more correct.Comment #90
wim leersNot sure what the best course of action is, but the simplest thing I can think of is to postpone this issue on the other. Sorry :(
Comment #91
xjmComment #92
David_Rothstein commentedI don't understand the change of direction in this issue - why should the internal storage needs of the Link module affect Drupal's developer experience, especially for people linking to things outside of that module?
base://.user-path://rather thanpath://orrelative-path://(based on earlier discussion in this issue)?path://oruser-path://(at least if it's developer-facing) would be really confusing in conjunction withpublic://andprivate://(as there's no indication which are file paths and which are non-file paths)?Comment #93
pwolanin commented@David_Rothstein - see the meta issue for discussion. Among other things, the stream wrapper API requires you to use ://, but now we will use
user-path:andbase:for the Link uri - this is more correct in therms of URI semantics, since :// indicates there is a host or authority.Comment #94
wim leersTo clarify: #93 refers to #2407505-39: [meta] Finalize the menu links (and other user-entered paths) system.
Comment #95
webchickI don't think David's questions were around the scheme itself, I think he's saying that this issue got hijacked from its original purpose which was making it easy for a developer to link to a path. (for certain use cases)
Comment #96
effulgentsia commentedRe #95: #2416843: Decide if a Url::fromPath() method and/or a Url::fromUri() scheme other than 'base' or 'user-path' is wanted to support contrib/custom modules
Comment #97
yesct commentedI see blocker tag added and then removed in #80 and #81, but @webchick mentioned in #2343669-20: Remove _l() and _url() that this is part of a chain of issues blocking a critical... does that make this critical?
Comment #98
pwolanin commentedDuplicate now to #2417333: Add support for user-path: scheme to Url class and #2416843: Decide if a Url::fromPath() method and/or a Url::fromUri() scheme other than 'base' or 'user-path' is wanted to support contrib/custom modules
Comment #99
David_Rothstein commentedI am concerned that by splitting this into two issues (and marking one critical and the other major) you've effectively chosen a solution... one which goes against most of the discussion in this issue. Because in the end we shouldn't have two APIs for the same thing, only one.
It doesn't totally bother me to split this up if it's blocking other stuff getting done; however, what I foresee happening is the second issue being ignored (because it's "only major", and no longer 100% necessary once a working API was added in the first issue, and we don't want to change APIs at this stage of the release cycle, and so forth etc). And then in the end, the developer experience is even worse...
How can we ensure that doesn't happen and that the second issue doesn't get postponed forever?
Comment #100
webchickYeah, I also am confused why the separate sub-issue was created instead of just amending the issue summary here, so we don't lose the discussion.
Comment #101
xjm@David_Rothstein, @webchick, we created a separate issue to make sure that the implementation details of #2412509: Change LinkItem.uri to type 'uri' rather than 'string' and introduce user-path: scheme didn't continue to derail the DX discussion here, and because there was a lot of confusing this issue with several others because of its long scope.
@David_Rothstein, I think that it's correct for the re-scoped DX issue to be major per the definition: https://www.drupal.org/core/issue-priority#major-tasks The issue should definitely be unpostponed when the blockers are resolved for further discussion once the rest of the API is complete so that the scope of the discussion is clear. Edit: the major issue might eventually be marked wontfix, depending on consensus, but let's discuss it over there.
Comment #102
David_Rothstein commentedLet's reopen this and return it to where it was in #82. In #2417459: Provide internal API for special schemes and thin public wrappers for user input and non-routed URLs there is some agreement to define methods as the public API for the user-entered/relative path situation (as well as some other related situations, like base://) and we already have patches here (up through #82) that do essentially that for the user-entered/relative paths.
Unfortunately, I think things have gotten more complicated in the interim, since 'user-path' has leaked into the public API on its own (not just as part of URL generation).
For example, in \Drupal\Core\Form\ConfirmFormHelper::buildCancelLink() there's this:
But then in standard_install():
Unless I'm missing something I don't think it's possible for the same method to replace both cases, because one is a URL and one isn't. I am not sure what to do about that.
Comment #103
wim leers#102
That was always the goal. Instead of distinguishing between
(route name, route parameters)pairs andpathandurl(for external URL), we now only haveuriin the link field (and hence also inShortcutandMenuLinkContent). Thebase:,user-path:,entity:androute:URI schemes are built-in URI schemes that Drupal knows how to resolve. They each have a specific purpose.Both of those are valid URIs. Both of those are of the form
user-path:<path like in D7>. You could simply view it as that: a URI-encoded way of representing paths. That's all it really is. Both of those can be resolved. So… what exactly do you think is impossible here?Comment #104
xjmWhy did we reopen this? The title is "Add a method to support UIs where users enter paths instead of route names and other valid use cases". We already added the API for "UIs where users enter paths" and we'll add a method as for the other schemes for better DX, in #2417459: Provide internal API for special schemes and thin public wrappers for user input and non-routed URLs. For the "and other valid use cases", there's #2416843: Decide if a Url::fromPath() method and/or a Url::fromUri() scheme other than 'base' or 'user-path' is wanted to support contrib/custom modules. For whether the name is correct, there's #2417567: Rename user-path: scheme to internal:. It's really frustrating to have to chase comments back and forth across so many issues.
Comment #105
David_Rothstein commentedI think #2416843: Decide if a Url::fromPath() method and/or a Url::fromUri() scheme other than 'base' or 'user-path' is wanted to support contrib/custom modules could be closed; all the patches and discussion for that are here.
OK, how about we postpone this issue on the discussion in #2417459: Provide internal API for special schemes and thin public wrappers for user input and non-routed URLs instead. It seems to me like replacing 'base:' with a method is going to be simpler than replacing 'user-path:' with a method (and therefore better as separate issues) but perhaps I am wrong, and if so I agree it is better to do both in the same issue. If we determine 'user-path:' is harder after all, we could reopen this.
Sadly, I have a feeling that replacing 'user-path:' with method calls is not going to work universally and therefore might wind up as "won't fix" but I would like to be wrong.
Comment #106
wim leersWe do have
\Drupal\Core\Url::fromUserInput()now, is there anything left here?Comment #107
wim leersPer #104 and #106 (no reply).