Change destination query string handling to break dependence on system path

Going to look at this.

This will almost certainly fail tests, but I want to see which ones

This should work more incrementally.

Leaving in a BC layer.

Testbot fluke.

I don't think we should use [] for the reason you mentioned, in case of an empty string.

Added/expanded/fixed some docs.

#2418219: Deprecate destination URLs that don't include the base path is the follow-up to fix all of the remaining usages.

https://www.drupal.org/node/2418229 is the change record.

Removing the config_translation hunk after discussion with @effulgentsia.
If we want to change getOverviewPath() to not return a path, we should rename it (or probably remove it altogether)

Adjusted the code formatting of the hunk in MenuForm.

This patch looks fine.

+1 to RTBC. This just has some docs changes, so leaving that status.

My biggest concern with this patch is basically every correlating + line to a - sign is completely different:

+++ b/core/modules/block_content/src/BlockContentListBuilder.php
@@ -39,7 +39,7 @@ public function buildRow(EntityInterface $entity) {
-      $operations['edit']['query']['destination'] = 'admin/structure/block/block-content';
+      $operations['edit']['query']['destination'] = $entity->url('collection');

+++ b/core/modules/user/src/Tests/UserAdminTest.php
@@ -52,7 +52,7 @@ function testUserAdmin() {
-    $link = $user_a->link(t('Edit'), 'edit-form', array('query' => array('destination' => 'admin/people')));
+    $link = $user_a->link(t('Edit'), 'edit-form', array('query' => array('destination' => $user_a->url('collection'))));

+++ b/core/modules/views_ui/src/ViewEditForm.php
@@ -727,7 +727,7 @@ public function renderDisplayTop(ViewUI $view) {
-          'query' => array('destination' => "admin/structure/views/view/{$view->id()}"),
+          'query' => array('destination' => $view->url('edit-form')),

Here we use call ->url() on an entity, and give it 'collection' even though the route name in the YAML file is entity.foo.collection.

+++ b/core/modules/block_content/src/Plugin/Menu/LocalAction/BlockContentAddLocalAction.php
@@ -26,7 +27,7 @@ public function getOptions(RouteMatchInterface $route_match) {
-      $options['query']['destination'] = 'admin/structure/block/block-content';
+      $options['query']['destination'] = Url::fromRouteMatch($route_match)->toString();

Here we use Url::fromRouteMatch(x)->toString();

+++ b/core/modules/comment/src/CommentManager.php
@@ -260,7 +261,7 @@ public function forbiddenMessage(EntityInterface $entity, $field_name) {
-        $destination = array('destination' => 'comment/reply/' . $entity->getEntityTypeId() . '/' . $entity->id() . '/' . $field_name . '#comment-form');
+        $destination = ['destination' => Url::fromRoute('comment.reply', ['entity_type' => $entity->getEntityTypeId(), 'entity' => $entity->id(), 'field_name' => $field_name], ['fragment' => 'comment-form'])->toString()];

Here we use Url::fromRoute()->toString();

Coming at this from a developer POV, what I did in D7 was look at my browser to see what URL I wanted to redirect to, copy/paste the path portion of that URL, stick it in an array('destination' =>foo) parameter, and be on my merry way.

In D8, I don't understand what I'm supposed to do. It seems like I need to be able to discern:

- Is the path I want to link to owned by an entity (has a route name like "entity.foo.blarg")
- If so, call $entity->url('last-part-of-route-name')
- If not, call Url::fromRoute()->toString(); except sometimes Url::fromRouteMatch()->toString();

:(

That's not only a huge amount of additional mental overhead for developers, it's also basically impossible to script through something like DMU unless I'm missing a cluestick.

So I'm not really comfortable committing this as-is. At the very least the change record needs updating because it only mentions one of the three+ things done here.

How about this? It's still 3 cases:

• $entity->url($relation)
• $this->url($route_name, $route_parameters)
• \Drupal::url($route_name, $route_parameters)

But, the difference between the last two is the same as we have elsewhere: are you in a class or in procedural code.

The difference between the first two is whether your destination is an operation on an entity or not. The concept of entities being different from non-entities is pretty fundamental to Drupal 8. See #2259445: Entity Resource unification for details about that in relation to the url() method. In any case, not invented by this issue.

Coming at this from a developer POV, what I did in D7 was look at my browser to see what URL I wanted to redirect to, copy/paste the path portion of that URL, stick it in an array('destination' =>foo) parameter, and be on my merry way.

That's a fair complaint, and seems like a topic for #2412709: Route names should have some kind of logical relationship to their canonical paths. Note comment 11 on that issue, which mentions web profiler as an example tool that can help.

Here's a few counter examples to consider though:

1. +++ b/core/modules/block_content/src/BlockContentListBuilder.php
   @@ -39,7 +39,7 @@ public function buildRow(EntityInterface $entity) {
   -      $operations['edit']['query']['destination'] = 'admin/structure/block/block-content';
   +      $operations['edit']['query']['destination'] = $entity->url('collection');
   

   Oh hey, I'm in a function where I'm given an entity. And where I want to redirect to is the listing page where that entity is a member of. Reading this code, I can understand that logic without happening to know that 'admin/structure/block/block-content' is that list.

2. +++ b/core/modules/block_content/src/Plugin/Menu/LocalAction/BlockContentAddLocalAction.php
   @@ -26,7 +28,7 @@ public function getOptions(RouteMatchInterface $route_match) {
   -      $options['query']['destination'] = 'admin/structure/block/block-content';
   +      $options['query']['destination'] = $this->url('<current>');
   

   Oh hey, I'm in the function that adds a local action to add a block from a list. And I want to add a destination parameter to that "Add" link so that after adding, I'm taken back to where I'm at now. Reading this code, I don't need to know what the URL of where I'm at now is.

3. +++ b/core/modules/user/src/Tests/UserAdminTest.php
   @@ -52,7 +52,7 @@ function testUserAdmin() {
   -    $link = $user_a->link(t('Edit'), 'edit-form', array('query' => array('destination' => 'admin/people')));
   +    $link = $user_a->link(t('Edit'), 'edit-form', array('query' => array('destination' => $user_a->url('collection'))));
   

   Oh hey, I'm already generating a link to the "edit-form" operation of $user_a. And I want to add a destination so that after editing that user, I'm taken to the listing page for users. Reading this code, I don't need to know where that listing page is.

Removing property declaration that is redundant with and conflicts with the trait.

The entity cases do look better to me.

The route cases not so much for the reason webchick states (i.e. we've gone from URL -> URL which you could figure out from the browser, to URL -> Route which you have to look in the routing YAML or otherwise debug for). That's a problem for any code-generated (and not user-entered) link though, so comes down to the relative value of having the system path and route names not being the same thing any more. Regardless of the actual value of that, trying to get everything so it works consistently now seems good. Having to use routes in some places and system paths in the other is the worst state I think.

Sorry, to clarify, my complaint was not about the annoyance of having to stop everything you're doing in your PHP file and go look up a machine name in some other file / UI / CLI / whatever. I know that ship has already sailed in D8. (Though it's kind of "funny" how many of us seem to be pretty "meh" on the cost/benefit of that change. :P)

My complaint was specifically about the fact that this patch chose no less than 4 different ways to replace what used to be a very simple string, and didn't sufficiently document what developers porting their modules were supposed to make of that.

Doing $something->url() every time is definitely better on the first concern, so thanks for that. However, the change record still looks the same so I don't think that second concern's been addressed yet https://www.drupal.org/node/2418229. We need something like #20 in there w/ before/after examples.

Then just a question:

+++ b/core/modules/block_content/src/BlockContentListBuilder.php
@@ -39,7 +39,7 @@ public function buildRow(EntityInterface $entity) {
-      $operations['edit']['query']['destination'] = 'admin/structure/block/block-content';
+      $operations['edit']['query']['destination'] = $entity->url('collection');

+++ b/core/modules/block_content/src/Plugin/Menu/LocalAction/BlockContentAddLocalAction.php
@@ -26,7 +28,7 @@ public function getOptions(RouteMatchInterface $route_match) {
-      $options['query']['destination'] = 'admin/structure/block/block-content';
+      $options['query']['destination'] = $this->url('<current>');

Why are those two going to the same place but using two different methods?

Thanks, much better!

And...

Committed and pushed to 8.0.x. Thanks!