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:

  1. As a developer, I want to handle user-entered paths, such as those coming from Menu UI, Link, or Shortcut modules.
  2. As a developer, I want to write automated tests that uses paths to simulate a user entering paths into a browser.
  3. As a developer, I want to create links to un-routed paths located within my Drupal site, such as README.txt or misc/druplicon.png.
  4. As a developer, I want to create links to un-routed paths located outside my Drupal site, such as /wordpress/admin.php.
  5. 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.
  6. 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

  1. Add support to Url::fromUri() for a user-path:// scheme and document the difference compared to base://
  2. 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.
  3. 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.

Comments

dawehner’s picture

Issue summary: View changes

It 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.

larowlan’s picture

Should it be deprecated too?

dawehner’s picture

Ideally 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.

aspilicious’s picture

This

It must be clear that this will not apply url aliases, unless enforced, language prefixes and what not.

is the reason why we should have this function, just to explain this.

dawehner’s picture

What do you think about this piece of documentation?

  /**
   * Creates a new Url object for a path relative to drupal base.
   *
   * Note: Instead of using this method you should use ::fromRoute, because of
   * the following reasons:
   *   - Path aliases and language prefixes aren't automatically applied.
   *   - The underlying paths could be changed, using route names ensured that
   *     you always point to the right location.
   *   - Using paths also makes it impossible to check access on them later.
   *
   * Beside of those reasons for things like tests it might be convenient to use
   * this method.
   *
   * @param string $path
   *   A path relative to the drupal base path.
   * @param array $options
   *  (optional) Options for the given path, see ::fromUri() for more details.
   * @return \Drupal\Core\Url
   *   A new URL object for the given path.
   */
  public static function fromPath($path, array $options = []) {
    $url = static::fromUri('base://' . $path, $options);
    return $url;
  }
aspilicious’s picture

The wording could be improved, but the message sounds ok to me. :)

dawehner’s picture

Status: Active » Needs review
StatusFileSize
new1.37 KB

Let's post a patch to attract people

pwolanin’s picture

We 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.

dawehner’s picture

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.

I 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 ...

effulgentsia’s picture

There'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/1

Say from here you want to a) output a link to the user/login "path", or b) output a link to the core/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.png

In 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).

effulgentsia’s picture

I'm concerned that this issue's approach of adding a Url::fromPath()

To 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.

dawehner’s picture

That is a fair argument. Maybe we could copy some sort of documentiation onto ::fromUri?

effulgentsia’s picture

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.

Part 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.
  • But ConfirmFormHelper's:
    $options = UrlHelper::parse($query->get('destination'));
    // @todo Use Url::fromPath() once https://www.drupal.org/node/2351379 is
    //   resolved.
    $url = Url::fromUri('base://' . $options['path'], $options);
    

    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://)?

Maybe we could copy some sort of documentiation onto ::fromUri?

+1. Perhaps after answering the above question on whether to add a front:// scheme?

pwolanin’s picture

Well, this looks wrong to me since we could process the destination path into a route and then render an absolute URL?

mpdonadio’s picture

StatusFileSize
new2.98 KB

At 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.

webchick’s picture

Issue tags: +Needs committer feedback

Not 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.

amateescu’s picture

#13 sounds to me like we want routed:// and unrouted:// schemas. At least those should be more self-descriptive than base:// and front://, but maybe it's just me :)

pwolanin’s picture

SO, 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.

pwolanin’s picture

Status: Needs review » Needs work

In other words - I think any approach using base:// for something that maps to a route needs work

dawehner’s picture

@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.

dawehner’s picture

Title: Add Url::fromPath() to improve DX » Add Url::fromDrupalPath() to improve DX
Status: Needs work » Needs review
StatusFileSize
new3.89 KB

Alright, let's be clear, what we deal with and shift the issue.

wim leers’s picture

  1. +++ b/core/lib/Drupal/Core/Url.php
    @@ -172,6 +172,32 @@ public static function fromRoute($route_name, $route_parameters = array(), $opti
    +   * Creates a new Url object for a path, which IS controlled by Drupal.
    

    I'd prefer *is* over IS, if only because api.d.o uses a font that makes it super difficult to distinguish between an uppercase i and a lowercase l.

  2. +++ b/core/lib/Drupal/Core/Url.php
    @@ -172,6 +172,32 @@ public static function fromRoute($route_name, $route_parameters = array(), $opti
    +   * This method is to be used for generating URLs where the exact source, so
    +   * route, is unknown, such as coming from user input.
    

    What about: This method should be used for generating URLs where the exact route is unknown. This should almost never be necessary, except when dealing with user input (we can't expect a user to know a route when creating a link; so we accept a path instead).

  3. +++ b/core/lib/Drupal/Core/Url.php
    @@ -172,6 +172,32 @@ public static function fromRoute($route_name, $route_parameters = array(), $opti
    +   * Drupal controlled paths, and Url::fromUri() for external paths, or paths
    

    s/Drupal controlled/Drupal-controlled/

  4. +++ b/core/modules/system/system.routing.yml
    @@ -383,6 +383,11 @@ system.theme_settings_theme:
    +# You can replace that route to improve UX.
    

    This route can be altered to improve UX; e.g. the controller can be replaced to provide a content creation UI.

  5. +++ b/core/modules/system/system.routing.yml
    @@ -383,6 +383,11 @@ system.theme_settings_theme:
    +    _only_fragment: TRUE
    

    I didn't know about this one :)

wim leers’s picture

I reviewed this, but I share this concern:

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() […]

mpdonadio’s picture

Can we mark this as an @internal?

wim leers’s picture

#24: ooh!

effulgentsia’s picture

+++ b/core/lib/Drupal/Core/Url.php
@@ -172,6 +172,32 @@ public static function fromRoute($route_name, $route_parameters = array(), $opti
+    return \Drupal::pathValidator()->getUrlIfValidWithoutAccessCheck($path) ?: Url::fromRoute('<invalid>');

Instead of an <invalid> route, should we throw an InvalidArgumentException?

effulgentsia’s picture

Title: Add Url::fromDrupalPath() to improve DX » Add Url::fromDrupalPath() to support the few cases where fromRoute() can't be used, such as user-entered paths
dawehner’s picture

@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.

dawehner’s picture

StatusFileSize
new4.02 KB
new1.59 KB

Thank you wim for your review!

I'd prefer *is* over IS, if only because api.d.o uses a font that makes it super difficult to distinguish between an uppercase i and a lowercase l.

Ha

I didn't know about this one :)

Still think its a hack, but this is how we made <none>to actually work as expected.

dawehner’s picture

I decided to not put @internalgiven that there are a limited set of usecases, also for contrib.

aspilicious’s picture

I'll have to buy beer (or other drinks) for everyone in this issue :)

webchick’s picture

Just 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.

amateescu’s picture

Building a bit on the schema suggestions from #17, how about fromRoutedPath() and maybe a sibling fromUnroutedPath()?

Edit: my keyboard ate the 'd' in fromUnroutedPath() above :)

dawehner’s picture

StatusFileSize
new4.02 KB
new11.36 KB
new8.27 KB

Building a bit on the schema suggestions from #17, how about fromRoutedPath() and maybe a sibling fromUnroutePath()?

I really like this idea!

On top of that I also replaced some of the existing bad usages of base://

Status: Needs review » Needs work

The last submitted patch, 34: 2405551-34.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
StatusFileSize
new11.32 KB
new949 bytes

Alright, you can't handle random paths with that.

pwolanin’s picture

I like webchick's suggestion of indicating a user input path in the method name.

effulgentsia’s picture

+1 to #32 / #37. What do you think of this as the implementation and docs of that?

pwolanin’s picture

We 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.

pwolanin’s picture

StatusFileSize
new11.56 KB
new8.59 KB

Here's sort of a merger of the approaches.

David_Rothstein’s picture

As a human who sometimes writes computer programs I like Url::fromUserEnteredPath() also... it's easy to understand.

+   * Keep in mind that the path of any Drupal route can vary from site to site.
+   * For example, a site could install a module that alters the canonical link
+   * template of nodes from 'node/{node}' to 'content/{node}' or that alters
+   * the path of the 'user.login' route from '/user/login' to 'login'.

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.

wim leers’s picture

+1 to #32 / #37.

+1

pwolanin’s picture

Title: Add Url::fromDrupalPath() to support the few cases where fromRoute() can't be used, such as user-entered paths » Add Url::fromUserEnteredPath() to support the few cases where fromRoute() can't be used, such as user-entered paths

ok, updating title

Also, @David_Rothstein yes - the code comment probably need some more work as you suggest.

effulgentsia’s picture

We should never use base:// here. If it can't be routed, it's a broken link

What if the user enters core/LICENSE.TXT or path/to/silex/page? You sure #38's fallback to base:// isn't what we want? See also #10.

dawehner’s picture

What if the user enters core/LICENSE.TXT or path/to/silex/page? You sure #38's fallback to base:// isn't what we want? See also #10.

That is valid, there might be usecases for it.

mpdonadio’s picture

Linking 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.

pwolanin’s picture

In 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.

dawehner’s picture

Well, 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.

pwolanin’s picture

So, 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.

mpdonadio’s picture

OK, 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

  • valid/routed/path => a routed URL object
  • bogus/path => a URL object representing the route
  • http://example.com/some/path => an external URL
  • base://some/path => an unrouted URL object

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.

effulgentsia’s picture

Title: Add Url::fromUserEnteredPath() to support the few cases where fromRoute() can't be used, such as user-entered paths » Add Url::fromUserEnteredPath() to support UIs where users enter paths instead of route names

As for whether we should use a version with base:// fallback, I think there are two reasons not to do it.

Here's some reasons to do it:

  1. In D7, I can navigate to /node/add/article?destination=README.txt and after submitting the form, get redirected to /README.txt.
  2. In D7 with the Link module, I can enter README.txt as the URL of a link field, and it will successfully link to that.
  3. If we want one particular UI, such as the Menu UI, to validate that a path corresponds to a Drupal route, #38 allows for that by having that UI call $url->isRouted(). There's even docs in that patch about that.
  4. In D7, the Menu UI does validate that, but I see no reason to carry that forward to D8. We said in #2407505: [meta] Finalize the menu links (and other user-entered paths) system that the common scenario of linking to a Drupal resource is with some kind of entity reference widget. Which means that for menus, typing in a path at all will be rare. And if that rarity is to support a use case of a menu link to blog being 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.
  5. Requiring the user to enter base://some_path if they want to link to a non-Drupal path vs. some_path if 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.
effulgentsia’s picture

And a delayed response to #41:

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.

I think it's important to distinguish between:

  • Changing a site's URL structure after it's already been built/deployed, such that there's content on the site that links to old URLs, links on 3rd party sites that link to old URLs, and people with browser bookmarks that link to old URLs.
  • Changing a new site's URL structure to suit the needs of the site rather than be stuck with Drupal's terminology and organization.

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 to url(). 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.

pwolanin’s picture

@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

dawehner’s picture

StatusFileSize
new11.56 KB
new2.37 KB

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.

Well, 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.

wim leers’s picture

Status: Needs review » Reviewed & tested by the community
StatusFileSize
new11.57 KB
new1.89 KB

This 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.

effulgentsia’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/lib/Drupal/Core/Url.php
@@ -202,7 +202,7 @@ public static function fromUserEnteredPath($path, array $options = []) {
-        ->getUrlIfValidWithoutAccessCheck($path) ?: Url::fromRoute('<invalid>');
+        ->getUrlIfValidWithoutAccessCheck($path) ?: Url::fromUri('base://' . $path);

Per #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.

+++ b/core/lib/Drupal/Core/Form/ConfirmFormHelper.php
@@ -34,9 +34,7 @@ public static function buildCancelLink(ConfirmFormInterface $form, Request $requ
       $options = UrlHelper::parse($query->get('destination'));
-      // @todo Use Url::fromPath() once https://www.drupal.org/node/2351379 is
-      //   resolved.
-      $url = Url::fromUri('base://' . $options['path'], $options);
+      $url = Url::fromUserEnteredPath($options['path'], $options);

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).

David_Rothstein’s picture

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).

That 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.

David_Rothstein’s picture

I think it's important to distinguish between:

  • Changing a site's URL structure after it's already been built/deployed....
  • Changing a new site's URL structure to suit the needs of the site rather than be stuck with Drupal's terminology and organization.

....
For the latter case, I don't think hook_url_(in|out)bound_alter() is optimal, either conceptually, functionally, or performance-wise...

That'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. Calling Url::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.

David_Rothstein’s picture

...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...

This 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

wim leers’s picture

If I want to link to the 'node/add' page in my code, calling Url::fromPath('node/add') is pretty easy to understand. Calling Url::fromRoute('node.add_page') is considerably more difficult.
[…]
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.

Unfortunately, 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.

David_Rothstein’s picture

This issue was originally about the following (https://www.drupal.org/node/2405551/revisions/8004803/view):

1) Add a Url::fromPath() that potentially calls uriFromRoute('base://...). This doesn't change any API but improves the DX a lot.
2) Add a giant docblock why and when to use it, and why this function is not prefered. This will also give us the possibilty to group all the needed info for the developers into one place.

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.

wim leers’s picture

Fair; 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 as Url::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 :).

berdir’s picture

I 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.

pwolanin’s picture

I think handling user input is the valid use case - let's stick with "fromUserEnteredPath"

effulgentsia’s picture

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.

What about fromSiteSpecificPath() or fromSitePath(). 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:

  • When dealing with user-entered text, since that is site-specific.
  • Tests. Since a test is in control of which modules and configuration it installs, so in that sense, is also site-specific, where by "site" I mean the test site.
  • Custom modules written for a single site, not written for sharing as a contrib module.
  • Contrib modules in their early stages of development. Until someone tries using it on a site that has a non-default path setup, at which point they can open an issue to convert to fromRoute() so that the module works for their site. Just like there are probably contrib modules out there that print out untranslated strings, until someone tries to use it on a non-English site for the first time and has to open an issue to call t().
webchick’s picture

Title: Add Url::fromUserEnteredPath() to support UIs where users enter paths instead of route names » Add a method to support UIs where users enter paths instead of route names and other valid use cases
Issue summary: View changes

Ok, 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.

effulgentsia’s picture

Issue summary: View changes

Added 2 more use cases based on #58 / #65. Wording might be a bit too confrontational, so please edit as needed.

webchick’s picture

Issue summary: View changes

Turning that into a numbered list so we can discuss them.

webchick’s picture

My 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.

David_Rothstein’s picture

What about fromSiteSpecificPath() or fromSitePath().

I 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.

webchick’s picture

Also, if #70 is the generally accepted feeling, then we should just go back to Url::fromPath() and be done.

joelpittet’s picture

+1 for #70 Url::fromPath()

effulgentsia’s picture

I 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 page http://example.com/drupal-subfolder/index.php/fr/node/add and 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 to http://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.

joelpittet’s picture

Also like @effulgentsia's suggestion of Url::fromRelativePath(). +1 to that or still cool with Url::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.

effulgentsia’s picture

Ok, 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:

  • If what's passed to the function is user/register, the resulting URL should be http://example.com/drupal-subfolder/index.php/fr/user/register
  • If what's passed to the function is en/user/register, the resulting URL should be http://example.com/drupal-subfolder/index.php/en/user/register
  • If what's passed to the function is custom-script.php, the resulting URL should be http://example.com/drupal-subfolder/custom-script.php
  • If what's passed to the function is ../custom-script.php, the resulting URL should be http://example.com/custom-script.php

So, arguments for fromRelativePath():

  • In the above examples, only the first one is what in Drupal 7, we called a "Drupal path" or "system path" or "internal path". The others are not. They are, however, all relative paths (and the implementation needs to figure out what it's relative to).

Arguments for fromPath():

  • pwolanin and others here might disagree that we should support all of the above, and argue that we should only support the 1st one.
  • Drupal/system/internal paths are (or will soon hopefully be) a legacy concept. Only Drupal developers from prior versions are burdened with their conceptual baggage. For web developers unburdened with that, "path" and "relative path" are pretty much synonymous, so "relative" doesn't disambiguate much, because the concept of "absolute path" is increasingly replaced with URIs.

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.

pwolanin’s picture

I think we need a set of schemes and at the API level only support fromUri().

The UI/widget may add a scheme.

aspilicious’s picture

I think we need a set of schemes and at the API level only support fromUri().

Just 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.

$url_token = [my-custom-url-token];
$url =  new Url::fromPath($url_token);

So *ïf* I understand your comment you're basically just ignoring everything I'm saying in the summary...
And than my code should be:

$url_token = [my-custom-url-token];
$url =  new Url::fromUri('base://' . $url_token);

Correct? (I hope not)

aspilicious’s picture

Talked with pwolanin

pwolanin: aspilicious_home: was just on a call w/ webchick and alex - we discussed defining a path:// scheme

That makes sense to me and is ok for me. And removes my concerns :)

catch’s picture

Not 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.

wim leers’s picture

Issue tags: +blocker

#2412509 is blocked on exactly this discussion:

pwolanin: aspilicious_home: was just on a call w/ webchick and alex - we discussed defining a path:// scheme

Depending on what we decide here regarding "paths", and how they should be stored (i.e. whether /node/1 is stored as node/1 or path://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.

effulgentsia’s picture

Ok, 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, path was the best scheme name we came up with, but I'm now recommending undetermined as 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 as routed:// 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:

public static function fromPath($path) {
  return static::fromUri('undetermined://' . $path);
}

@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.

pwolanin’s picture

Status: Postponed » Needs review
StatusFileSize
new11.65 KB

re-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.

pwolanin’s picture

Title: Add a method to support UIs where users enter paths instead of route names and other valid use cases » Add a URI scheme to support UIs where users enter paths instead of route names and other valid use cases
Issue summary: View changes

From the other issue - possibly settled on a pass trying to implement as user-path://

pwolanin’s picture

StatusFileSize
new13.7 KB
new11.82 KB

Ok, here's a quick pass at the approach of using a URI.

Status: Needs review » Needs work

The last submitted patch, 84: 2405551-84.patch, failed testing.

catch’s picture

Issue tags: -Needs committer feedback

If 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.

pwolanin’s picture

Status: Needs work » Needs review
StatusFileSize
new13.75 KB
new1.07 KB

oops, forgot to make the route parameters as arrays in the test.

larowlan’s picture

Status: Needs review » Reviewed & tested by the community
StatusFileSize
new1.58 KB
new13.74 KB
  1. +++ b/core/lib/Drupal/Core/Url.php
    @@ -216,8 +219,14 @@ public static function fromRouteMatch(RouteMatchInterface $route_match) {
    +   * Note: the user-path:// scheme should be avoided except when processing actual
    

    nitpick > 80 :(

  2. +++ b/core/lib/Drupal/Core/Url.php
    @@ -216,8 +219,14 @@ public static function fromRouteMatch(RouteMatchInterface $route_match) {
    +   * Url::fromRoute() for code linking to any any Drupal page.
    

    one to many any

  3. +++ b/core/lib/Drupal/Core/Url.php
    @@ -225,12 +234,21 @@ public static function fromRouteMatch(RouteMatchInterface $route_match) {
    +      $path = substr($uri, 12);
    

    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

wim leers’s picture

Oh, 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 use user-path:// (currently used in both that and this patch) or user-path:, which is more correct.

wim leers’s picture

Status: Reviewed & tested by the community » Postponed

Not 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 :(

xjm’s picture

Issue summary: View changes
David_Rothstein’s picture

I 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?

  1. Using a scheme here is pretty odd DX compared to simply calling a method - barely better than base://.
  2. Why would it be user-path:// rather than path:// or relative-path:// (based on earlier discussion in this issue)?
  3. What happened to the documentation of the other ways this might be used (i.e. the "site-specific" concept from above)?
  4. Aren't we worried that any new scheme such as path:// or user-path:// (at least if it's developer-facing) would be really confusing in conjunction with public:// and private:// (as there's no indication which are file paths and which are non-file paths)?
pwolanin’s picture

@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: and base: for the Link uri - this is more correct in therms of URI semantics, since :// indicates there is a host or authority.

wim leers’s picture

webchick’s picture

I 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)

yesct’s picture

Issue summary: View changes
Issue tags: +blocker

I 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?

David_Rothstein’s picture

I 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?

webchick’s picture

Yeah, 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.

xjm’s picture

@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.

David_Rothstein’s picture

Title: Add a URI scheme to support UIs where users enter paths instead of route names and other valid use cases » Add a method to support UIs where users enter paths instead of route names and other valid use cases
Status: Closed (duplicate) » Needs work

Let'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:

  $url = Url::fromUri('user-path:' . $options['path'], $options);

But then in standard_install():

  $shortcut = entity_create('shortcut', array(
....
    'link' => array('uri' => 'user-path:node/add'),
  ));
  $shortcut->save();

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.

wim leers’s picture

#102

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).

That was always the goal. Instead of distinguishing between (route name, route parameters) pairs and path and url (for external URL), we now only have uri in the link field (and hence also in Shortcut and MenuLinkContent). The base:, user-path:, entity: and route: URI schemes are built-in URI schemes that Drupal knows how to resolve. They each have a specific purpose.

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.

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?

xjm’s picture

Why 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.

David_Rothstein’s picture

Status: Needs work » Postponed

I 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.

wim leers’s picture

Status: Postponed » Active

We do have \Drupal\Core\Url::fromUserInput() now, is there anything left here?

wim leers’s picture

Status: Active » Closed (duplicate)

Per #104 and #106 (no reply).