Make Url::toString(TRUE) explicit by having a Url::generate() method, as well as having GeneratedUrl::toString

+++ b/core/includes/form.inc
@@ -860,7 +860,7 @@ function batch_process($redirect = NULL, Url $url = NULL, $redirect_callback = N
-        return new RedirectResponse($batch_url->setAbsolute()->toString(TRUE)->getGeneratedUrl());
+        return new RedirectResponse($batch_url->setAbsolute()->toGeneratedUrl());

+++ b/core/lib/Drupal/Core/Url.php
@@ -730,6 +730,16 @@ public function toString($collect_bubbleable_metadata = FALSE) {
+   * @return \Drupal\Core\GeneratedUrl
...
+    return $this->toString(TRUE);

Should it return the generated URL (string) or the GeneratedUrl (object)? I think ->toGeneratedUrl()->getGeneratedUrl() makes very little sense

Yep, tim.plunkett make sense,

+  /**
+   * Generates a object representation with the a rendered URL and metadata.
+   *
+   * @see \Drupal\Core\Url::toString
+   *
+   * @return \Drupal\Core\GeneratedUrl
+   */
+  public function toGeneratedUrl() {
+    return $this->toString(TRUE);
+  }

function toGeneratedUrl() returns "->toString(TRUE)", updated patch with "->getGeneratedUrl()".

Yep, that makes sense. I just think that GeneratedUrl::getGeneratedUrl() is a terrible method name, but that's out of scope here.

I think this is only slightly better. It's still very confusing/bizarre.

If everybody thinks this is a step forward, then I'm fine with it. I'm fine with anything, as long as we continue to have cacheability metadata for generated URLs. No matter what we do, it's always going to be lipstick on a pig.

I agree all of this is ugly, confusing, bizarre, suboptimal, and so on. Let's not forget the root cause though: our URL generator, and Symfony's, is infested with globals.

Okay so I'm fine with the addition of toGeneratedUrl()

But is there a reason to even call ->toGeneratedUrl()->getGeneratedUrl() in any of these cases in the patch? How is it different than just ->toString()?
Or does it just indicate a place where the metadata isn't properly handled, and should never exist?

Regardless, <code>->toGeneratedUrl()->getGeneratedUrl() is just too confusing to read. Proposing deprecating getGeneratedUrl() for toString().

Better summing up my reasoning for that change:

IMO when we have a value object that is just "a string + cacheable metadata", it should have a ->toString() method

Also, whenever we have a call to Url::toString(), it probably means that metadata isn't handled properly, and is likely a bug. But thats out of scope here.
Fixing a bug, bad merge added the method twice.

+++ b/core/lib/Drupal/Core/Url.php
@@ -734,6 +734,16 @@ public function toString($collect_bubbleable_metadata = FALSE) {
+  public function toGeneratedUrl() {

Why not generate() instead?

Url::generate() then results in a GeneratedUrl() object.

That seems more logical than Url::getGeneratedUrl().

<timplunkett> WimLeers: $url->toGeneratedUrl()->getGeneratedUrl() is what really killed me

This is not in the IS nor title.

Agreed with the rename

Was that supposed to be an RTBC?
Rerolled.

I think for the 1st point we can use @trigger_error('Deprecation message', E_USER_DEPRECATED) which will be more meaningful.

Addressed #22 and fixed more usages.

🎉

There's a lot of weirdness here. But just trying to get it passing first...

Testbot issue (selenium didn't load).
Retest passes

Looks ready except

+++ b/core/lib/Drupal/Core/GeneratedUrl.php
@@ -22,9 +22,22 @@ class GeneratedUrl extends BubbleableMetadata {
+   * @deprecated in Drupal 8.3.x, will be removed before Drupal 9.0.0.
...
+    @trigger_error('Deprecated in Drupal 8.3.x, will be removed before Drupal 9.0.0. Use \Drupal\Core\GeneratedUrl::toString().', E_USER_DEPRECATED);

8.7.x I guess...

Given a \Drupal\Core\Url object, there are now two non-deprecated methods to call.
toString(), if you just want the string and don't care about cacheable metadata
generate(), if you do need the cacheable metadata, and later you can call toString() on this object instead of the original Url object.

IMO, everyone should always be calling generate() and using that result to include cacheable metadata, and then go on to get the string.
I'm not 100% sure what the use case is for bypassing that, barring laziness. (perhaps too harsh of a word, but meaning that the concept of caching is bypassed here).

Furthermore, there are two instances in this patch where the code does $url->generate()->toString(), which tricks the system into thinking the dev has respected the cacheable metadata, when they haven't. This avoids the dreaded exception that says

The controller result claims to be providing relevant cache metadata, but leaked metadata was detected. Please ensure you are not rendering content too early.

via \Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber::wrapControllerExecutionInRenderContext().

I left those hacks in there to get the tests to pass. But is that okay?

I'm not 100% sure what the use case is for bypassing that, barring laziness. (perhaps too harsh of a word, but meaning that the concept of caching is bypassed here).

AFAIK there's two reasons:

1. unintentional: you're coming from old code and are wanting to make minimal changes, including not trying to understand D8 concepts (minimizing cognitive burden while porting)
2. intentional: you just want to call toString() because you hate/don't care for cacheability

That's also why we did that automatic bubbling in the background in D8, to ensure people actually did think about it.

If I understand your proposal, then I think you're saying that going forward:

1. use Url::generate() if you care about cacheability (you get a value object with the generated URL and its cacheability)
2. use Url::toString() if you don't care about cacheability (you get a string), cacheability is still bubbled automatically in the background, and could still result in that dreaded exception

IOW: this keeps the same automatic bubbling. Do we want to? After years in D8, can we drop \Drupal\Core\Render\MetadataBubblingUrlGenerator and leave that responsibility with the developer? So that if they use Url::toString(), no cacheability is bubbled in the background. That'd remove magic. But it'd also increase reponsibility.
Perhaps this is out-of-scope for this particular issue, but it made me think of this.

Perhaps it would be overly prescriptive or result in even more ->generate()->toString() hacks, but why not deprecate Url::toString() as well to encourage handling of metadata?

Definitely works for me. Now that you mention it … I now recall that that was the whole reason we did it this way: we couldn't break existing code.

Doing what you describe there would also allow us to deprecate \Drupal\Core\Render\MetadataBubblingUrlGenerator. And there's be fewer code paths. And less magic.

I think this might be too much. But let's see how bad it is.

Sloppy attempt at find and replace, let's see.

core/modules/image/templates/image-formatter.html.twig giving some trouble (and Twig in general, thanks to \Drupal\Core\Template\TwigExtension::escapeFilter() having toString calls)

It would certainly be easier to review. My worry is that if we actually split it up, we'd be forcing people to change from
$url->toString(TRUE)->getGeneratedUrl()
to
$url->generate()->getGeneratedUrl()
to
$url->generate()->toString()

That feels rather odd (I especially dislike the generate()->getGeneratedUrl() part).
But honestly this patch is completely unreviewable as-is, so we probably have to do something.

My interest for now is trying to get it passing first, before splitting up.

I guess that's better than doing them in the other order, where the intermediate API would call for
$url->toString(TRUE)->toString() ;)

After some LogicException pain and various tries to understand what is happening inside the Url code... I wanted to open an issue to add extra method documentation to Url::toString() to ease some developer pain, but then came across this one. I assume this issue is still current / not superseded by anything.

This change feels sensible. I'll try to look at it with fresh eyes when everything has sunk in but as far as I can see now:
* People may see it as a DrupalWTF that they have to do $url->generate()->toString() to get a simple string
* ...but they won't have to debug any LogicExceptions
* ...and we're likely able to explain the reasons. (I can't totally get my head around the concepts of Url vs UrlGenerator vs GeneratedUrl yet, but I'll try again later.)

So: picking this up.

First a 'straight' reroll. It wasn't trivial but I'm fairly sure I didn't miss anything.
Tomorrow I'll
* add more replacements. (Even in the context of the patch I see some lines that used to be $entity->url(), were changed to $entity->toUrl()->toString(), and now need to be changed to $entity->toUrl()->generate()->toString().)
* hopefully look at what the testbot says.
* maybe later look at "deprecate \Drupal\Core\Render\MetadataBubblingUrlGenerator". It would be an honour.

Interdiff is boring; it's only

• replacements of ->toString() by ->generate()->toString()
• replaced usage of $url by $generated_url in ~3 places for consistency.
• and one deletion of a toString() call somewhere in a test that should have been working on a GeneratedUrl in the first place.

I've seen 'something' is needed in TwigExtension::escapeFilter() / theme_render_and_autoescape(), but will first see what the bot says.

Of course this failed to apply against #1883744 - rebasing.

Here's one with the TwigExtension::escapeFilter() stuff - the first 'interesting' interdiff I've uploaded.

I will have questions, because I'm not so sure anymore that just doing a sweeping replacement by ->generate()->toString is really... adding anything useful.
But those questions will possibly wait until the patch is green, and split out in two, as per #49. (Because the questions are about 'phase 2' anyway.)

While gathering thoughts, I've picked apart the patch into 4 parts. I'm hoping the first 3 will succeed, in which case I'll have a more solid basis for writing up thoughts.

Well... gathering thoughts took a while. I have them drafted now, but let's see if this goes green first, before I comment on it.

I'm rebasing this patch in several separate commits on my local system, so I can re-add things if needed (and it's a little easier for me to keep chasing HEAD if I have to). I can also upload separate patches for s/toString(TRUE)/generate() and s/getGeneratedUrl()/toString()/ for easier review if you want.

The one uploaded as #64 is basically equal to #31, except:

• more changes in jsonapi which I'm not including in an interdiff because they are simple s/toString(TRUE)/generate() and s/getGeneratedUrl()/toString()/
• Added some inconsequential variable renames, for consistency
• Removed the change to the toString() in rdf.module; moved it into #2335487: Remove the multiple comment optimization from rdf.module
• Backed out some s/generate()->toString()/toString()/ replacements which were also backed out in #45
• Added comments in the docblock for Uri::toSring() and Uri::generate().

That is: I backed out the deprecation of Url::toString(FALSE), because I don't 'get' it. (I'm still keeping a rebased copy of that code locally, though.)

This is now a fairly straightforward replace of one function by another, plus a docblock addition (and I'm going to argue for moving discussion about other things into a followup issue).

And...

I'm fully aware that me barging into a Daniel/Tim/Wim issue, holds a risk of just creating diversion, but... I don't have another option if I want to help out on the Url related issues. (I tried to do my homework and read up.)
...and to me it feels like the discussion went sideways from #35 - #39, or I'm just not getting things.

---

[#35 / Tim] IMO, everyone should always be calling generate() and using that result to include cacheable metadata, and then go on to get the string.

Before I get to talking about things I don't get, let's just first enumerate where Core itself doesn't do this, but calls plain Url::toString() - which can be found by searching the '1.2.3.4-62' patch (mostly equal to Tim's latest patch) that replaced them with ->generate()->toString():

• Docs mentioning to use Url::fromUri()->toString() for things. (Which basically means we've been teaching people to do the wrong thing?) This includes token.api.php.
• TwigExtension / theme_render_and_autoescape()
• the Batch API (which is probably OK, I don't know details)
• a few install functions (which is probably OK)
• Lots of tests. (I have no idea whether that's fine.)
• Lots of theme preprocess functions
• Lots of hook_help implementations
• comment_tokens(), node_tokens(), user_tokens(), views_tokens()
• A few #description / #markup / etc entries in forms
• A few #actions in forms
• some hook_page_attachments / alter functions
• A few views plugins' render() methods
• A few views style plugin's attachTo()
• A few field formatter plugins' viewElements() methods
• A media source plugin's getMetadata()
• ContentUninstallValidator
• RenderElement::preRenderAjaxForm() rendering a 'progress path'
• EntityReferenceEntityFormatter::viewElements()
• StreamWrapper\TemporaryStream::getExternalUrl()
• TempStore\Element\BreakLockLink::preRenderLock()
• BlockContentController::add() (returning render element)
• NodeViewController::view() (returning render element)
• LinkGenerator::generate() (only for external links so that's OK)
• BlockContentAddLocalAction::getOptions()
• block_place_toolbar()
• comment_entity_view()
• CommentLinkBuilder::buildCommentedEntityLinks()
• CommentManager::forbiddenMessage()
• contact_mail()
• ContentTranslationHandler::entityFormSharedElements()
• EntityDisplayFormBase::form() / submitForm()
• FilterPermissions::permissions()
• ForumUninstallValidator:validate()
• LayoutBuilder::buildAdministrativeSection()
• media_requirements()
• media_form_field_ui_field_storage_add_form_alter()
• media_field_widget_multivalue_form_alter()
• _media_get_add_url()
• MediaTypeListBuilder::buildRow()
• media/src/Plugin/media/Source/OEmbed - several methods
• MenuForm::buildOverviewForm()
• NodeTypeListBuilder::getDefaultOperations()
• NodeSearch plugin's prepareResults()
• UserSearch plugin's execute()
• rdf_comment_storage_load() (but I'm addressing that in #2335487)
• settings_tray's BlockEntitySettingTrayForm::buildForm()
• system_user_login()
• views_views_pre_render()
• RedirectResponse returned by:
  
  Entity\Form\DeleteMultipleForm
  
  FormSubmitter::redirectForm()
  
  CommentController::commentApprove()
  
  layout's AddSectionController::build()
  
  MediaDeleteMultipleConfirmForm::getConfirmText
  
  user module's AccessDeniedSubscriber::onException() and MaintenanceModeSubscriber::onKernelRequestMaintenance()

That's... a fairly long list. (It's longer than when Tim last looked at it because Entity::Url() was deprecated, which unhides a lot of these cases where generate() was not called.)
Also: there's the ~400 lines of \Drupal::url() which I haven't fully looked at yet but will soon also be toString() methods, as per #2731817: Replace all calls to the deprecated Drupal::url() function in Core.

I'm not sure if I 'get' verything involved in the above code. From what I can gather, just doing toString() without calling generate() may be OK:

• if the code is sure it is being executed inside a render context and as long as 'metadata bubbling in the background' stays in place
• or if the code is returning a RedirectResponse and the response is always supposed to be non-cacheable.

...but 1) I have a hard time believing that's the case in all the above code, and 2) then the 'bubbling in the background' can't be removed, right? (Or do we not always care about handling metadata, because e.g. we completely trust that other code in the same request adds the same metadata? I don't believe that's our stance.)

---

[#38 / Tim] Perhaps it would be overly prescriptive or result in even more ->generate()->toString() hacks, but why not deprecate Url::toString() as well to encourage handling of metadata?

Here's where I was losing the discussion.

1)

I was first assuming that we were going to just replace toString() calls by straight generate()->toString() calls, since patch #42 and further were doing that. But AFAICT we cannot do this because it introduces a risk of cache pollution. And it encourages bad practice. (Because we're not handling the cacheability metadata. Or -same question as above- are there instances where we don't care about handling these, because e.g. we completely trust that other code in the same request adds the same metadata?)

So for now I am going to assume that we don't want to do a straight replace, and we should fix the majority of code in the above list, to properly handle metadata. But that seems like a separate followup (or even meta issue?) because it's too much to just fix alongside the original intent of this issue.

Feel free to tell me whether I should approach this differently... or if separate issues should indeed be opened to fix up Url::toString() code in Core. I can do some work on this if needed.

2)

Personally, yes I am indeed afraid that this is going to result in even more ->generate()->toString() hacks. I am afraid that deprecating Url::toString() isn't going to encourage handling of metadata.

Not only because Core itself isn't always giving the right example and I don't think there is explicit wording in source code / drupal.org telling people to do the right thing yet... but in my experience, for some years the prevalent advice linked from Google has been to do the wrong thing: "just use ->generate()->toString()".

I'm not saying we shouldn't eventually deprecate toString() and/or remove automatic bubbling... but I doubt we're ready yet (see above) / I'm fairly convinced we should move the discussion about that to a followup issue. It's too big/hairy to decide halfway the comments on this issue.

IMHO the goal of this issue is to improve DX - in two ways:

1) renaming the two methods to be more intuitive, as per patch #31

2) document toString() to tell people about its restrictions and proper usage, which I added a proposal for (and which was my original reason for bumping into this issue). Because AFAICT that has not really been done before - and is a necessary step on the path toward perhaps deprecating toString(). (See also 'soapbox rant' below if you want.)

[#37 / Wim] IOW: this keeps the same automatic bubbling. Do we want to? After years in D8, can we drop \Drupal\Core\Render\MetadataBubblingUrlGenerator and leave that responsibility with the developer? So that if they use Url::toString(), no cacheability is bubbled in the background. That'd remove magic. But it'd also increase reponsibility.

Similar answer: AFAICT we are not ready for dropping MetadataBubblingUrlGenerator and putting the responsibility on all developers to do the right thing, because from my viewpoint, we have not properly started telling people that they have that responsibility. (Again see 'soapbox rant' below if you want.)

I'm sure many contrib developers who call Url::toString() don't know what it does in the background and what they should be thinking of. Telling people more explicitly would be the first/next step on a path toward deprecating automatic bubbling. Or... we can start telling them and deprecating toString() right now, but IMHO at least

• that needs its own separate issue for clearer discussion.
• AFAICT we have followup work to do on Core, before we can do this'; see above.

Again - I can do at least some work on this if we have agreement / you tell me what is the way forward.

---

'Soapbox rant':

Just "generating a URL (string) that represents a route in Drupal" seems like a simple thing for developers; noone naturally understands or assumes they heed to read the caching API and know what a cache context and bubbleable metadata is, just to be able to do this.

I get that we can't (always) make "just outputting a simple URL" easy. Because caching issues. But then we should at least tell them "this is not easy, because caching issues".

I imagine an average developer is like me in 2017 (#3032255: Extend Cacheable/RedirectResponse constructor to accept GeneratedUrl): if they want to create a URL for something, are going to look at the Url object. And then find a method toString(). Well,

• Guess how many times the substring "cach" appears in the Url object: Zero.
• An average developer won't know what "bubbleable metadata" is, or that it has anything to do with caching. It's just not mentioned anywhere.
• So then they're going to look around and try to understand the code behind Url and UrlGenerator and MetadataBubblingUrlGenerator and GeneratedUrl, and they're likely just going to get lost; reading that code doesn't give any clarity either.
• A good number of people are then going to run into the frustrating "early rendering / leaked metadata" exception, and have no idea that this has anything to do with just creating a simple URL
• And in the end, after some suffering/searching, many of those people are going to assume that the solution is to use toString(true)->getGeneratedUrl(). Because that's the only information they found. (For years, the highest ranking informative google pages in a google search were https://drupal.stackexchange.com/questions/187086/trustedresponseredirec... and #2630808: Response::getResponse() does not collect cacheability metadata from Url object. The former leads to the latter, where an experienced Drupal developer is saying to use toString(true)->getGeneratedUrl().)
• ...and they won't ever know that they have introduced risk of cache pollution, because they didn't see anything telling them this.

I'm not blaming the code for being complex, but on the way from "I just want to output a URL" to "hey there's a URL object" to "WTF is this early rendering exception I'm getting" to (hopefully not but somewhat likely) "just use toString(true)->getGeneratedUrl() and be done with it"... at least one road sign is missing.

Which explains the positively monstrous addition to the method docs of Url::toString(), that I'm putting up for review / in the interdiff. For people who have not been exposed that they need to think about cacheability, we IMHO need to have some practical pointers specific to Url generation, without them being absolutely required to read cache API docs. And I don't think these are anywhere else.

I'm almost sure this size of comment breaks at least the spirit of code comment standards, but... it's just a sign of the complexity / interdependencies we've introduced. And I think these kinds of pointers are suitable for a method docblock.

I'm happy to be told that this is the wrong way to go about things, or that the description is wrong, or whatever. I'm also happy to split this out into another issue if this is considered separate from this issue. But in my mind the order of things to happen in this area is:

1) this issue renaming Url::toString(TRUE) and UrlGenerator::getGeneratedUrl(), which I'm happy about as a simple developer.

2) at least one more road sign for devs trying to "do URL strings" (possibly in this same issue)

3) creating followup issue(s) about deprecating toString() and/or the automatic metadata bubbling and/or cleaning up Core code that needs to be done first.

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.