Problem/Motivation

Currently UrlGenerator::generateFromPath() is marked as deprecated but there are various proper usecases:

  • Strip bad protocols for external URLs
  • Add query parameters and fragments for external URLs
  • Provide an URL to a "internal" (relative/absolute) url but not controlled by the Drupal routing system.

It is not supposed to be used for the following, which is why it was originally marked @deprecated.

  • Generate an internal URL given a raw system path, because the only valid way to that in 8.x is to use a route name and arguments.

There are alternatives to use for generating internal URLs, but we do not have anything for external/non-Drupal URLs at all.

Furthermore, url() (and therefore l()) still rely exclusively on UrlGenerator::generateFromPath() in all cases. \Drupal::url() behaves differently and properly takes a route name as the main argument.

Proposed resolution

Option 1. Remove the deprecation marker and update the documentation to use generateFromRoute in cases you have a Drupal internal URL.

This will lead to incorrect code in contrib and custom modules that pass system paths to url() - since the system path is not at all guaranteed to continue working and there'll be no formal @deprecated warning, only an informal recommendation.

Option 2. Separate the external URL code to a separate method somewhere, and keep generateFromPath() (and url()) deprecated.

Option 3. Separate the external URL code to a seperate method somewhere, and completely remove generateFromPath and url()

Option 4: remove all outbound path processing from this method as well as the 'alias' option. So if you call url('node/1') you get http://example.com/node/1 - no path prefixes, no aliases. This makes url() handle all the valid cases, but not handle the invalid use case at all.

Option 5 (only chx suggested this): postpone the routing system to Drupal 9. It doesn't provide any benefits and the amount of technical debt created is not resolvable within a reasonable timeframe.

Also, since catch marked #2278559: url() function docs have vanished? as a duplicate of this issue, the patch for this issue must also include either removing or adding param/return documentation to the url() function in common.inc.

Remaining tasks

User interface changes

API changes

CommentFileSizeAuthor
#2 generate_from_path-2339219-2.patch1.23 KBdawehner

Comments

catch’s picture

Priority: Normal » Critical

Moving to critical, there needs to be a safe, recommended way to handle external URLs before an RC even if it's just un-deprecating this function.

dawehner’s picture

Status: Active » Needs review
StatusFileSize
new1.23 KB

Here is a first start.

Crell’s picture

Issue tags: +WSCCI

This is probably for the best given the circumstances. But we should note in the comment that using this method for Drupal paths is a security hole since it bypasses all of our route-based logic, and strengthen the encouragement to use routes wherever possible.

damien tournoud’s picture

Feeding an external URL to a method called generateFromPath() sounds extremely weird.

wim leers’s picture

Feeding an external URL to a method called generateFromPath() sounds extremely weird.

+1

  • Strip bad protocols for external URLs
  • Add query parameters and fragments
  • Provide an URL to a "internal" url but not controlled by the Drupal routing system.

If these are the valid use cases, then perhaps we should rename generateFromPath($path, $options) to manipulateUrl($url, $options)?

Provide an URL to a "internal" url but not controlled by the Drupal routing system.

Can you give examples of this?
If we can remove this use case, then we can change it to manipulateExternalUrl($url, $options), which would be clearer. And it'd be a strong signal that to generate a Drupal URL, you must now use routes, period. That'd be nice :)

dawehner’s picture

I would suggest to keep generateFromPath but have one dedicated generateFromExternalUrl, so to be more consistent with the other methods.

Can you give examples of this?
If we can remove this use case, then we can change it to manipulateExternalUrl($url, $options), which would be clearer. And it'd be a strong signal that to generate a Drupal URL, you must now use routes, period. That'd be nice :)

Some people certainly want to use Drupal for everything, but well, I doubt this is a good idea. Random example: you have a REST server using silex under the same domain but a different path prefix. foo.example.com/* is drupal and
foo.example.com/api is your service. You still want to be able to link to it.

wim leers’s picture

foo.example.com/* is drupal and
foo.example.com/api is your service. You still want to be able to link to it.

People doing this surely are smart enough to write an FooApiUrlGenerator? Why is that not good enough?

dawehner’s picture

People doing this surely are smart enough to write an FooApiUrlGenerator? Why is that not good enough?

really?

I also don't see somone want to rewrite all the url() method we still have in core.

wim leers’s picture

I didn't word it very well, but I don't see why Drupal should support linking within the same domain that's not a Drupal-powered link and still treat that as an "internal" URL. Why can't that be treated like any other external URL? Drupal can't generate the link for sure. So you end up with a "hardcoded" URL anyway, just like when linking to any other site on the internet.

Otherwise "internal" and "external" lose their meaning. "internal" then is no longer "the Drupal site" but "anything on the current domain". In the case of a single domain with many blogs in subdirectories, that'd feel rather bizarre. Isn't it simpler to just treat "internal" as "the Drupal site" only?

Perhaps I'm not understanding you correctly :(

dawehner’s picture

Well, in general Drupal still uses url() for internal URLs, so we certainly cannot just drop the functionality. Sure, most of them can be probably replaced.

In generla you want to provide path, query parameters and maybe a fragment and generate a URL. This is a generic functionality, whether this is for external URLs, drupal internal URLs, or URLs under the same domain, so certainly
useful.
Drupal though, especially in D8, will talk with various services, which might just exists under a different PORT.

In general I don't see why we should limit features which already exists and are useful

wim leers’s picture

I agree with everything you wrote in #10.

I just thought we wanted to get rid of url() before D8 ships, but apparently that's not the case. There are 415 remaining calls, 335 of which (81%) are in tests. Wouldn't it be doable to just remove all of those?

Once that's the case, what I wrote in #5 makes more sense:

Perhaps we should rename generateFromPath($path, $options) to manipulateUrl($url, $options)?
[…]
If we can remove this use case, then we can change it to manipulateExternalUrl($url, $options), which would be clearer. And it'd be a strong signal that to generate a Drupal URL, you must now use routes, period. That'd be nice :)

Crell’s picture

manipulateUrl() sounds like an alter hook. What does "manipulate" even mean?

dawehner’s picture

Wouldn't it be doable to just remove all of those

Well, for sure we can try, started a dedicated issue for that, but yeah we still need support for them in places like the batch system: #2340251: Remove most remaining url() calls

Once that's the case, what I wrote in #5 makes more sense:

Still disagree, but well.

tim-e’s picture

Just thought I would provide a recent experience with this issue. I ran into it issue when working on #306662: Add redirect option to site-wide contact forms.

We want users to be able to choose a path to redirect to after they submit the contact form. Ideally I would have liked this to allow them to enter either an internal path or external url, but was advised that it might be a security issue to allow an external url so just decided to stick with an internal path. Anyway, the main issue I ran into was that I had been using UrlGenerator::generateFromPath, which worked exactly as we needed it to. However when I noticed that it was deprecated I decided to look at other options. The problem I had is all the other options provide the path with a leading slash. This causes the #type => 'path' form element to fail validation and thus wouldn't work. So in this case, it seems that we need users to be able to enter a path without a leading slash.

Crell’s picture

By this point most things should be using a path with leading slash, no? That's what all of the new code is built on... If we haven't actually done the job of converting to that then we've kinda screwed ourselves. :-)

larowlan’s picture

@Crell the issue is PathValidator->getUrlIfValid hard-codes the leading '/' in the request - ie:

    $request = Request::create('/' . $path);
    $attributes = $this->getPathAttributes($path, $request);

Would you support a patch to that to only do so if it didn't already exist in $path?

@larowlan

larowlan’s picture

catch’s picture

That's not the right change, rolled back and re-opened #2340379: Allow PathValidator::getUrlIfValid to support paths with leading /.

xjm’s picture

Title: Remove deprecation from generateFromPath and update documentation. » Finalize URL generation API (naming, docs, deprecation)
Issue summary: View changes
Issue tags: +beta target, +Pre-AMS beta sprint

Retitling since the previous title sounded like anything but a critical issue. (Even with the expanded scope it seems more like a major to me since it's still possible to do everything in D8 that we could in D7, just a little confusing.)

Also tagging as a beta target because url() itself is a very commonly used function, and if we do want to deprecate, remove, or replace usages of that function, we should do so soon. I'd in fact consider removing url() beta deadline, and maybe this whole issue as well if we're renaming methods or changing expectations.

manipulateUrl() is a far worse name than generateUrlFromPath(). If it takes an internal path OR external URL I guess it would technically be generateUrlFromPathOrUrl(), which is essentially what url() has always been anyway.

If it takes an external URL only, it would be something like sanitizeUrl() or generateExternalUrl(). That would be less magical and have less chance of being abused.

However, we shouldn't expect end users to provide route names, so matching an internal path to a route name and then generating a URL from it is also a very common usecase. If we want it to be magical and figure what to do with a variety of inputs, maybe it should internally do a route match and then use generateFromRoute()?

pwolanin’s picture

Yes, url() should be marked deprecated for sure or better removed by beta

wim leers’s picture

generateExternalUrl() > manipulateUrl(), absolutely. My only point was that we should remove "path" from the name if it's only about external URLs. And since all internal URLs should be route-based, not path-based, that should be the case.

However, we shouldn't expect end users to provide route names, so matching an internal path to a route name and then generating a URL from it is also a very common usecase.

Shouldn't we? And should we really still allow internal paths to be used? If we allow that, then IMO we might as well have stuck with paths everywhere. Two different representations for the same concept is going to be very confusing.

dawehner’s picture

However, we shouldn't expect end users to provide route names, so matching an internal path to a route name and then generating a URL from it is also a very common usecase.

I disagree 200% with this statement. We worked REALLY hard to use route names all over the place. We do have a good tools to discover route names (https://www.drupal.org/project/webprofiler)
etc.. The recommended way for really internal URLs should be route names.

@Wim Leers
Maybe something like validateUrl or ensureSafeUrl($url) would be a good naming?
Other usecases are certainly JS frameworks which also maybe require URLs to something without a HTTP route on the Drupal backend but just in the framework (ember, angular etc.)
Drupal is not a insular solution, sorry, we need to accept that.

wim leers’s picture

Other usecases are certainly JS frameworks which also maybe require URLs to something without a HTTP route on the Drupal backend but just in the framework (ember, angular etc.)
Drupal is not a insular solution, sorry, we need to accept that.

This is definitely true. But…

  1. I'm pretty sure that JS frameworks will not want to make calls to the server to determine the URLs to… talk to the server :)
  2. We already have Drupal.url() … which indeed mirrors url() on the server side, which means it works … yep, with paths. There's a big difference with the server side though: A) routes aren't readily available in JS, B) using routes is expensive, see point 1.

Looks like we actually agree :) Do we have an issue to remove all remaining calls to url()?

jhodgdon’s picture

Issue summary: View changes

catch marked #2278559: url() function docs have vanished? as a duplicate of this issue, so just adding to the summary that the patch for this issue must also include either removing or adding param/return documentation to the url() function in common.inc.

dawehner’s picture

I'm pretty sure that JS frameworks will not want to make calls to the server to determine the URLs to… talk to the server :)

But isn't there a chance that Drupal templates have to maybe at least partially render those URLs? But yeah we should try to remove as many of the calls as possible, see #2340251: Remove most remaining url() calls

xjm’s picture

Title: Finalize URL generation API (naming, docs, deprecation) » [meta] Finalize URL generation API (naming, docs, deprecation)
Status: Needs review » Active

Let's make this critical a meta issue, since there's a lot of specific parts to it. Wim found #2340251: Remove most remaining url() calls for me. Edit: And I see now that @dawehner linked it in the last comment, xjm--.

I'm struggling with the fact that this seems beta-blocking. Either using url() as it always has been is supported and best practice, (and therefore UrlGenerator::generateFromPath() is also best practice for all url()'s usecases), or one of the most commonly used functions in Drupal should be deprecated or removed and doesn't have clearly documented replacements for all cases. (\Drupal::url() only works for internal routes.) And l() still calls url().

xjm’s picture

webchick’s picture

Can we get an updated issue summary that talks about what we're proposing here? The current one makes it sound like this is a simple docs issue, and it's obviously ballooned from there.

catch’s picture

Issue summary: View changes
catch’s picture

Issue summary: View changes
catch’s picture

Issue summary: View changes
dawehner’s picture

So I really like option 4, together with bumping up #2340251: Remove most remaining url() calls as every call to url() with an internal URL will be a bug after that (not language support).

larowlan’s picture

Note #2342949: Cleanup BlockContentBlock found an incorrect use in BlockContentBlock, but will get rid of it there.
+1 to Option 4

jhodgdon’s picture

+1 to option 4. With adequate documentation of course. :)

xjm’s picture

With option 4, what happens to the expected behavior of l(), which still calls url() and still takes paths instead of routes? How do we explain to developers the implications of url() (and then maybe l()) essentially not using the routing system? How do we avoid confusion between \Drupal::url() and url(), which sound like the same thing but are not as the former acts on routes and calls a different method entirely?

wim leers’s picture

I like option 4 also, but… won't that still make it "too easy to do things wrong"? Many contrib developers only use English, and in that case, url() will seem to provide correct results, but only in their situation. Also, we then have both url() and \Drupal::url(), which appear very similar but are vastly different (i.e. xjm's concerns).

-1 to option 4 if it means having both url() and \Drupal::url(), with those names, and those functionalities.


However… doesn't removing url() imply removing l() also? It's the same old system that the routing system has been working to get rid of.

The confusion between \Drupal::url() and url() is not new. It was introduced in #2078285: Add short-cut methods to the \Drupal class for generating URLs and links from routes. It's supposed to make things "as simple as url() again", AFAICT. But yes, it will seem confusing at first, if you assume it behaves the same as url(), because it takes very different parameters. OTOH, this makes it very clear that "the old" is a global function that takes paths, and the new is a global *shortcut* to something that should actually be injected and accepts route names and route parameters.


Therefore I think that if we go with something like option 4, we still want:

  1. something like url(), but named differently
  2. keep \Drupal::url() as-is
  3. remove l() and add something like \Drupal::link(), which also uses routes, just like \Drupal::url()

UPDATE: my comment was extremely confusing, rewritten.

catch’s picture

So for l() I'd make that behave the same as url() yes.

We already have LinkGenerator::generate() for generating links from routes, which is equivalent to UrlGenerator::generateFromRoute() (although differently named, of course).

Wim's right that it'd be easy for people to use the function incorrectly - more or less by not porting old. I'm not sure that it's a requirement to completely break people's code though if they're not paying attention - the worst that happens here is a broken link, and in both Drupal 6 and Drupal 7 we had a lot of API changes that didn't blow up if you didn't port properly ( both watchdog(), url() etc. it was possible for old code to be buggy without exploding).

I'd be OK with Wim's Option 4a which boils down to 'rename something so people have to make a conscious choice when upgrading', just not sure how bad it is if we don't do that and make the other changes here.

Coder etc. could have rules to look for calls that start with user/node/admin etc. which are common Drupal internal paths and report those as warnings.

Not averse to a rename, but don't think it's a blocker. The confusion between Drupal::url() and url() is worse, although that's also already there really.

catch’s picture

xjm’s picture

I'd also prefer to rename url() regardless. It will cause people a lot of pain when upgrading if the same function starts doing only a part of what it did before, because "broken links a lot of places but not always" is the sort of frustrating silent failure that would make me, as a contrib developer or site owner, stabby.

wim leers’s picture

because "broken links a lot of places but not always" is the sort of frustrating silent failure that would make me, as a contrib developer or site owner, stabby.

+1000

Explicit failure > silent failure.

That's the thing Drupal <8 did, and it was painful and infuriating, every single time.

xjm’s picture

I'd also like to spell out that changing what a function does without changing its name is still a BC break.

xjm’s picture

Issue summary: View changes
xjm’s picture

I agree with Wim's proposal. We already have \Drupal::l() parallel to \Drupal::url(). So I'd suggest the following steps:

  1. Convert all uses of url() (and any incorrect uses of generateFromPath()) to the correct replacement in #2340251: Remove most remaining url() calls. Aside from the zinger in l(). There are 300-some per that issue
  2. Convert all uses of l() to \Drupal::l() or whatever else fits in the usecase. @alexpott checked and there are 100-some remaining, mostly in help text where a conversion to a route name is straightforward.
  3. Remove l() and url(), with a very clear change record and very clear docs.
  4. Rename generateFromPath() to something that more correctly describes correct uses, and remove incorrect ones (as in Option 4 in the summary). This might include splitting it into generateExternalUrl() and something else.
  5. Document all of the above.

Steps 1-3 are beta-blocking in my estimation.

xjm’s picture

Also, the change record needs to include examples of what to do if your D7 code can take either an internal or external URL, and there need to be docs and @see on \Drupal::l() and Drupal::url() to the other methods that were part of what l() and url() did in Drupal 7 and before.

alexpott’s picture

We also need to deprecate and remove UrlGeneratorInterface::setBaseUrl(), UrlGeneratorInterface::setBasePath() and UrlGeneratorInterface::setScriptPath() since #2342925: Simplify url() in order for the installer to function without it removed the last runtime usages and they make no sense to support. Also we probably should try and remove UrlGenerator::updateFromRequest().

xjm’s picture

Discussed with @Dries, @catch, @alexpott, @webchick, @effulgentsia, and @jhodgdon. Documenting the correct way to generate internal and external URLs is beta-blocking, and at a minimum renaming l() and url() to _l() and _url() if we cannot remove all uses correctly. @webchick suggested doing the two renames first, and @catch pointed out that (as per the original scope of the issue) we need to hve a supported, documented way of generating external URLs.

xjm’s picture

Issue tags: +beta blocker
xjm’s picture

Issue tags: -beta blocker

Moving the beta blocker tag to the three parts of this issue that are clearly beta-blocking:
#2343651: Remove most remaining l() calls
#2340251: Remove most remaining url() calls
#2343661: Rename l() to _l() and url() to _url(), and document replacements

We can continue to discuss what to do for external URLs and the edgecases described in the summary. So long as we clearly document the known replacements for url() and l() in a majority of cases, other improvements are less critical to have completed for the first beta.

effulgentsia’s picture

Rename generateFromPath() to something that more correctly describes correct uses, and remove incorrect ones (as in Option 4 in the summary). This might include splitting it into generateExternalUrl() and something else.

How about renaming generateFromPath() to generateFromInternalPath(), making it only have logic for internal paths (by which I mean, paths handled by index.php), and leaving it as @deprecated with a goal to remove by 8.0.0. And then adding a new method, generateFromUrl(), that receives a Drupal\Core\Url object? That can be used both for situations where you know you have an external path (by which I mean, either a relative or absolute URL not handled by index.php) as well as for situations where code must be polymorphic for internal routes and external URLs (since the Url object supports both).

And yes, I realize the circularness in a method named UrlGenerator::generateFromUrl(), but I think that's the price we pay for having a class named Url, when that word is already defined by the internet, as a string.

effulgentsia’s picture

And yes, I realize the circularness in a method named UrlGenerator::generateFromUrl()

Also, callers wouldn't need to call it like that. They could do something more like Url::createFromPath($external_path)->setOptions($options)->toString(), which already internally calls the generator.

catch’s picture

Do we need one more beta blocker that's "provide an API function to replace url()/l() for external urls" - then #2343661: Rename l() to _l() and url() to _url(), and document replacements would only be the rename and replacement.

@effulgentsia I think we need code that specifically only deals with external URLs and doesn't attempt things like outbound path processing or aliasing at all - then it'll be clear to use that vs. generateFromRoute().

Then if there needs to be a method that's agnostic about what it gets, it can just check external or not and act accordingly.

effulgentsia’s picture

Do we need one more beta blocker that's "provide an API function to replace url()/l() for external urls"

Opened #2343759: Provide an API function to replace url()/l() for external urls.

I think we need code that specifically only deals with external URLs

Agreed, and incorporated that into the proposed resolution of that issue.

Then if there needs to be a method that's agnostic about what it gets, it can just check external or not and act accordingly.

Yep. And it can use the existing \Drupal\Core\Url class to assist with that if it wants to.

chx’s picture

Issue summary: View changes
Crell’s picture

So... what's the current proposal? I'm lost on the bits and pieces mentioned here now, as they seem to be discussing a lot more than just "make url() only do some things" (aka option 4).

xjm’s picture

We discussed this set of issues today with @xjm, @effulgentsia, @pwolanin, and @WimLeers, and came to the following consensus. (The following high-level summary may be incomplete or have errors, but I wanted to make sure it's documented publicly as soon as possible.) We also discussed it with @dawehner afterward and he agreed to this consensus as well.

  1. In general, creating a link requires a link text and a URL. Having different methods of generating a link depending on the source of its URL is confusing and overwhelming. To resolve this, we've agreed to do #2277103: Switch Drupal::l() and LinkGenerator to expect a Url object in order to provide a consistent, grokkable API.
  2. There are two overall categories of URLs in Drupal: routed and non-routed. Right now, we have a variety of disparate APIs for these categories depending on the context. In order to make the API very clear and easy to use with \Drupal::l(), Url will provide two static methods: Url::routed() and Url::unrouted().
  3. Url::routed() will provide the same behavior as the current constructor for Url, using the url_generator service to create a Url object given a route name and parameters. For now, this will wrap the constructor. Later, we will convert uses of the constructor to this method for a consistent API (and we may make it protected).
  4. Url::unrouted() will provide a parallel solution for #2343759: Provide an API function to replace url()/l() for external urls, using a new unrouted_url_assembler service to create a Url object given a URI as in igloo. This URI-as-in-igloo must begin with a scheme:// For base-URL-relative, non-routed paths like /robots.txt, /update.php, etc., the special custom base::// is used. If Url::unrouted() is called without a scheme, it will throw an exception, and the exception will clarify that a scheme is required, and that most pages "within Drupal" should use Url::routed() instead. This will significantly reduce the chance that it is implemented for e.g. node/1 (resulting in links that circumvent the routing system).
  5. We will also add a Url::setUnroutedUrlAssembler() method similar to Url::setUrlGenerator(), so that this service can be overridden on a per-instance basis as well as globally.
  6. We will deprecate and/or remove redundant or unsupported methods from LinkGenerator and UrlGenerator.
  7. We did not finalize as a group what exactly to do with \Drupal::url(), but the remaining work will build on top of the work already done in #2340251: Remove most remaining url() calls.

So, the final API will look something like this:

$link1 = \Drupal::l($link_text, $whatever_url_object);
$link2 = \Drupal::l($link_text, Url::routed('system.admin'));
$link3 = \Drupal::l($link_text, Url::unrouted('base://robots.txt'));
$link3 = \Drupal::l($link_text, Url::unrouted('http://othersite.example.com/foo'));

This slightly increases verbosity in some cases (but in a self-documenting rather than overwhelming way), greatly reduces it in others, and creates single, distinct concepts for links and URLs that are less likely to be confused with D7 concepts. It also treats links, URLs, and routing as the separate concepts that they are, rather than jumbling the three in various combinations.

In followup (non-beta-blocking) issues, we may:

  1. Convert file_create_url() to Url::unrouted('base://...')
  2. Convert core uses of new Url() to Url::routed() and then possibly protect the constructor.
  3. Add a method on entities for creating the "normal" link to the entity: #2345793: Entities should provide a method for a properly generated link to the entity
  4. Support a host::// scheme for non-base-url relative paths on the current host.

I am going to work on updating the issue summary. @WimLeers is working on the LinkGenerator side of things in #2277103: Switch Drupal::l() and LinkGenerator to expect a Url object, and @pwolanin is working on the Url side of things in #2343759: Provide an API function to replace url()/l() for external urls (which will either be rescoped or split into a couple different issues, TBD).

dawehner’s picture

Crell’s picture

I talked this over with xjm. After some explanation I am on board with this approach, as it basically forces everyone into using domain objects (good), and separates route-based from non-route-based links (good).

I would suggest, at least, fromRouted() and fromUnrouted(), to make the method names more self-documenting as factories. I am not comfortable with something as common as url generation having a static method as its primary interface but at least at the moment I don't have a much better suggestion. However, I want to flag this as something that will bite us later on testability and reserve the right to say "I told you so". :-)

xjm, Peter, and I also briefly discussed Drupal::l(). Since, again, we want to drive people toward injected services as much as possible l() should be kept in sync with LinkGenerator, so there's no reason to use it other than "I'm in a hook and therefore I can't inject things". While it's not the end of the world if we don't correct that it's a rather big DX fail if we don't.

jhodgdon’s picture

RE #55 - seems great! Note that in several of your examples, you used Url::routed() where it should have been Url::unrouted(). Oh, wait, you fixed them already. :)

dawehner’s picture

pwolanin’s picture

Examples need to be fixed since the method names were changed in the patch

fabianx’s picture

Generally the proposal looks great to me, but:

> 1. Convert file_create_url() to Url::unrouted('base://...')

That will not be possible as it is a completely different domain.

file_get_url is meant to rewrite URLs to go for example via a CDN, while your link should probably go not ...

What about:

- Url::route('node.get', array()) or Url::createRoute('node.get')
- Url::path('base://') or Url::uri('base://') or Url::external('base://') or Url::create('base://') or Url::createUrl() or simply Url::url()

plus:

- Url::fileUrl() or Url::createFileUrl() or Url::getFileUrl()

instead of routed / unrouted and that what is missing ...

Thanks!

tim.plunkett’s picture

This IS is out of date. We ditched ::unrouted() and ::routed() for ::fromUri() and ::fromRoute().

fabianx’s picture

Okay, nice. Thanks, Tim! :)

Then my point is only:

> 1. Convert file_create_url() to Url::unrouted('base://...')

That will not be possible as it is a completely different domain.

file_get_url is meant to rewrite URLs to go for example via a CDN, while your link should probably go not ...

So we need:

Url::fromFile()

and that should be pretty consistent.

jbrown’s picture

I think \Drupal\Core\Url needs a __toString() magic method: #2350507: \Drupal\Core\Url has no __toString() magic method.

fabianx’s picture

fabianx’s picture

An open question that remains is:

- How do I link to an alias?

( The answer could be an alias route - as proposed in #2346189: Denormalizing paths into route names/parameters is brittle / broken)

But wanted to hear if there was any thought that had been going into this already?

fabianx’s picture

Okay, it is Url::fromUri('base://myalias') I would use that to link to files, but never though to use that as an alias ...

fabianx’s picture

And it turns out in the beta 1 rush, we completely forgot to update the theme layer as that still has only:

url(),path() routes like symfony plus extra options and url_from_path (now probably needs to stay as we are in beta).

But there is no Url::fromUri() we can use the same from theme and module layers, maybe url_from_path is enough, though that one is schema-less.

pwolanin’s picture

fabianx’s picture

hass’s picture

I finally have a DX fail by the many url() changes and do not see any light. Docs seems not understandable to me, too.

These documentation looks useless to me either.

I need to generate the path /drupal8/search/node?search=foo, if I'm inside search.

Example code just for demo with debugging (go to core search and search for a keyword "test"):

if (strpos(\Drupal::routeMatch()->getRouteName(), 'search.view') === 0) {
  // the value will be 'node'
  $search_path = \Drupal::routeMatch()->getParameter('entity')->getPath();
  // Does not work, result: /drupal8/search?search=foo - Why is this not working?
  $test = Url::fromUri('search/' . $search_path, array('query' => array('search' => 'foo')))->toString();
  kint($test);
  // Does not work, result: /drupal8/search?search=foo - Why is this not working?
  $test = \Drupal::url('search.view', array('entity' => $search_path), array('query' => array('search' => 'foo')));
  kint($test);
  // Works. Result, result: /drupal8/search/node?search=foo
  $test = Url::fromUri('base://search/' . $search_path, array('query' => array('search2' => $keys)))->toString();
  kint($test);
  // Works, but is deprecated. Result: /drupal8/search/node?search=foo
  $test = \Drupal::urlGenerator()->generateFromPath('search/' . $search_path, array('query' => array('search' => 'foo')));
  kint($test);
}
  • I suspect I should not use generateFromPath().
  • In past we had one url() and now we have... 4-5 strange behaving functions? Do we really need this DX fail?
  • I do not understand why I need base:// in Url::fromUri()
  • I do not understand why I cannot use base:// in \Drupal::url().
  • I do not understand why \Drupal::url('search.view', array('entity' => $search_path)) does not work as this is a routed path and I guess that 'entity' is the right property and 'node' the correct value. Documentation - fail.
  • Url::createFromPath('http://drupal.org') one more strange function that seems to behave like Url::fromUri()

This is a DX and I guess others will run into the same. No idea what the right way is now...

larowlan’s picture

I think you need
Url::fromRoute('search.view_node_search', [], ['query' => ['search' => 'foo']])->toString()

See Drupal\search\Routing\SearchPageRoutes lines 78 => 99.
It adds a distinct route for each entity_id with the entity_id specified in the _defaults instead of in the path.
If you look at search.page.node_search.yml you can ascertain it serves the search/node path.

fabianx’s picture

Added related issue where we discuss some of the restrictions of the current proposal here.

hass’s picture

@larowlan: Thanks for your helpful hints. That's really unbelievable hard to understand without your help. I'm asking me how I could make this flexible for other entity types. Just using Url::fromRoute('search.view_' . $entity_id . '_search', [], ['query' => ['search' => 'foo']])->toString() may work for node and user, but it looks not very unreliable for other use cases. A contrib module could define it's own entity and name the route 'search.foo_' . $entity_id . '_mycustomsearch' and it will never match again. I guess that is not solvable :-(

jhodgdon’s picture

@hass -- Did you look at the SearchPageRoutes class?

What it is actually doing is looking at the list of active search pages defined in the admin/config/search/pages UI. Each one of these is a search page entity, and it is assigned a machine name by an admin. The route machine name is then "search.view_$entity_id", where $entity_id is that machine name.

It happens that the default shipped config for User and Node searching has the machine names set to user_search and node_search, but you cannot count on it.

What you can count on is that if you do a loop like what the SearchPageRoutes class is doing:

    $active_pages = $this->searchPageRepository->getActiveSearchPages();
    foreach ($active_pages as $entity_id => $entity) {
        $route = "search.view_$entity_id";
        // DO SOMETHING HERE WITH THE ROUTE
   }
}

this will find all of the active Search page routes, assuming they are using the Core search pages/plugin functionality.

Anyway this is probably off-topic for this issue.

hass’s picture

@jhodgdon: Yes, I've seen this, but what is $this in hook_page_alter()? I may optimize something on http://cgit.drupalcode.org/google_analytics/tree/google_analytics.module... .

jhodgdon’s picture

@hass - I don't understand your question, and probably this discussion should go elsewhere.

jhodgdon’s picture

On another note, it seems that l() has now been removed. But the only change notice I found just says it's deprecated. This needs to be fixed ASAP -- if there is a new change notice, I didn't see it, and the old one is not accurate any more.
https://www.drupal.org/node/2346779

hass’s picture

url() and l() still exists, but have been renamed to _url() and _l(). That may cause the confusion... both renamed functions are marked deprecated. I guess this should intentionally break old code.

fabianx’s picture

Another potential problem:

base:// could leak into the URI space for media, but there is no corresponding stream wrapper. This will likely affect contrib.

Having a valid uri like base:// that we cannot resolve via a stream wrapper might easily lead to problems and confusion.

EDIT: Edited post to remove wrong information.

chx’s picture

Note that this issue blocks everything link related -- the LinkItem is plain broken (install Drupal in a subdir, enter link, edit) and it has seen 0 comments in two weeks.

michaellander’s picture

I added a child issue pertaining to removing HTML attribute responsibility from Drupal\Core\Url:

#2385373: \Drupal\Core\Url objects should not be responsible for attribute handling for \Drupal\Core\Link

Would like maintainer feedback.

filijonka’s picture

Been reading through this issue now to see if what is done and not done and based #55 the only things I can find that still needs to have new issues

1. Convert file_create_url() to Url::unrouted('base://...') (renamed to fromUri)
2. Convert core uses of new Url() to Url::routed() and then possibly protect the constructor. (renamed to fromRoute)

wim leers’s picture

#83.1: see #61 and #63 regarding why that's not desirable.

filijonka’s picture

thanks @Wim Leers, did miss #63

so if I get this right there is a need for a fromFile()?

#83.2 stands as it is then?

webchick’s picture

Not sure if we can close this one out yet, but adding it to the mega plan.

effulgentsia’s picture

Priority: Critical » Major

The only critical child of this issue is #2343669: Remove _l() and _url(), which I think we can track without this meta. This meta is still useful for tracking the remaining non-critical work, so downgrading to Major accordingly.

wim leers’s picture

Status: Active » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.