Replace all _url() calls beside the one in _l()

#2364157: Replace most existing _url calls with Url objects looks like it's covering Views already. Is this child issue still necessary?

This issue was filed as a follow up for the _url issue.

If it's a follow-up it should probably be marked postponed. Doing so, and updating the issue summary.

Its independent, why would it have to be postponed on it?

Avoiding conflicts?

Avoiding conflicts?

How? we convert code which uses _url() calls which are in different contexts that other _url() functions.

For the record, this is still valid even after #2364157: Replace most existing _url calls with Url objects. Updated the issue summary with a current grep. (Sorry, I don't know how to make it fancy-looking like mpdonadio had it.)

Let's try to work on it a bit.

@mpdonadio and @dawehner pair programmed on that.

Update issue summary

1. +++ b/core/modules/views/src/Plugin/views/display/DisplayPluginInterface.php
   @@ -0,0 +1,534 @@
   +/**
   + * Created by PhpStorm.
   + * User: dawehner
   + * Date: 30/01/15
   + * Time: 14:14
   + */
   

   :)

2. +++ b/core/modules/views/src/Plugin/views/display/DisplayPluginInterface.php
   @@ -0,0 +1,534 @@
   +interface DisplayPluginInterface {
   

   I think DisplayRouterInterface should extend this now

3. +++ b/core/modules/views/src/ViewExecutable.php
   @@ -1696,15 +1708,28 @@ public function buildTitle() {
   +      throw new \InvalidArgumentException('You cannot create a URl to a display without routes.');
   

   URL, not URl

4. +++ b/core/modules/views/src/ViewExecutable.php
   @@ -1721,41 +1746,40 @@ public function getUrl($args = NULL, $path = NULL) {
   -            $pieces[] = '*'; // gotta put something if there just isn't one.
   ...
   +          $parameters[$variable_name] = '*';
   

   Where is our wonderful comment? Also, how is Url going to behave with this?

5. +++ b/core/modules/views/tests/src/Unit/ViewExecuableTest.php
   @@ -0,0 +1,174 @@
   +   * @var
   

   There are a bunch of these

6. +++ b/core/modules/views/views.theme.inc
   @@ -331,11 +332,16 @@ function template_preprocess_views_view_summary(&$variables) {
   +      $url = Url::fromUri('user-input:' . $base_path);
   

   user-path:, apparently (here and elsewhere)

7. +++ b/core/modules/views/views.tokens.inc
   @@ -132,7 +133,7 @@ function views_tokens($type, $tokens, array $data = array(), array $options = ar
   +        $replacements += $token_service->generate('url', $url_tokens, array('path' => $url->getInternalPath()), $options);
   

   Isn't getInternalPath deprecated? ;)

Isn't getInternalPath deprecated? ;)

That one is tricky ... sadly ... we literally want the path, as the o

.

Rerolled the patch.

@RavindraSingh: Rerolled which patch? The file size is very different from other patches.

Rerolled the patch.

Thank you for trying to.
Did you maybe mixed something up? The patchsize decreased etc. and there was no commit in the meantime, so well.

I'll try yo fix the failures

@mpdonadio was working on it.

I'll pick this up again.

Note that #18 is not a correct patch.

Filed #2417865: Add a Views DisplayPluginInterface to make the patch scope more manageable.

Fixing a bunch of unit tests and starting to fix PathPluginBase.

Silly typo.

1. +++ b/core/modules/views/src/Plugin/views/display/Feed.php
   diff --git a/core/modules/views/src/Plugin/views/display/PathPluginBase.php b/core/modules/views/src/Plugin/views/display/PathPluginBase.php
   index 7738933..075bbc5 100644
   
   index 7738933..075bbc5 100644
   --- a/core/modules/views/src/Plugin/views/display/PathPluginBase.php
   
   --- a/core/modules/views/src/Plugin/views/display/PathPluginBase.php
   +++ b/core/modules/views/src/Plugin/views/display/PathPluginBase.php
   
   +++ b/core/modules/views/src/Plugin/views/display/PathPluginBase.php
   +++ b/core/modules/views/src/Plugin/views/display/PathPluginBase.php
   @@ -490,11 +490,20 @@ public function validate() {
   
   @@ -490,11 +490,20 @@ public function validate() {
       * {@inheritdoc}
       */
      public function getUrlInfo() {
   -    if (strpos($this->getOption('path'), '%') !== FALSE) {
   -      throw new \InvalidArgumentException('No placeholders supported yet.');
   -    }
   -
   -    return Url::fromRoute($this->getRoute($this->view->storage->id(), $this->display['id']));
   +    return Url::fromRoute($this->getRouteName());
      }
    
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function getRouteName() {
   +    $view_id = $this->view->storage->id();
   +    $display_id = $this->display['id'];
   +    $view_route_key = "$view_id.$display_id";
   +
   +    // Check for overridden route names.
   +    $view_route_names = $this->state->get('views.view_route_names') ?: array();
   +
   +    return (isset($view_route_names[$view_route_key]) ? $view_route_names[$view_route_key] : "views.$view_route_name");
   +  }
   

   Note: We have a unit test in PathPluginBaseTest

2. +++ b/core/modules/views/views.theme.inc
   @@ -935,7 +947,7 @@ function template_preprocess_views_view_row_rss(&$variables) {
   -  $variables['link'] = check_url($item->link);
   +  $variables['link'] = $item->link;
   

   Note: $item->link contains the rendered and stripped url already.

3. +++ b/core/modules/views/views.tokens.inc
   @@ -132,7 +133,7 @@ function views_tokens($type, $tokens, array $data = array(), array $options = ar
   -        $replacements += $token_service->generate('url', $url_tokens, array('path' => $path), $options);
   +        $replacements += $token_service->generate('url', $url_tokens, array('path' => $url->getInternalPath()), $options);
   

   ... getInternalPath() is bad but its exactly what we want here. I think its fine for this patch ... getting rid of _url() and _l() is a good thing.

Views is full of crying.

Here is an interdiff which fixes quite some of the remaining failures.

I have not reviewed the changes in #33 entirely, but I did see one change that kind of conflicted with another approach I was trying. I will try to incorporate changes in #33 in the next patch but I just wanted to put this to the test for now.

I have used diff from @dawehner. Lets see the test bot results.

I am just summarizing the differences in patches in #34 and #35 (which is from an interdiff in #33).

+++ b/core/modules/views/src/Form/ViewsExposedForm.php
@@ -115,7 +117,7 @@ public function buildForm(array $form, FormStateInterface $form_state) {
-    $form['#action'] = _url($view->display_handler->getUrl());
+    $form['#action'] = $view->display_handler instanceof DisplayRouterInterface ? $view->getUrl()->toString() : Url::fromRoute('<current>');

This is from the interdiff in #33 and in patch in #35. This looks somewhat different from the previous approach. The reason this failed in previous patch (which called getUrl() on the $view directly) was that it did not handle linked displays the way it was handled before (via getPath() method).

In the patch in #34, I wrote a new method getRoutedDisplay() which does the same thing for displays and used it along with getPath() call (which we should be able to remove after refactoring, maybe a follow-up).

@hussainweb
You made a good point with the link displays!

@dawehner: Thanks! I am now stuck with views which don't have any linked displays at all. Previously, _url() function would return the <front> path if the path is null. Now, we are throwing an exception.

I am thinking of catching the exception in ViewsExposedForm and using Url::fromRoute('<front>') for that. I am trying a local test first and then posting a patch. Does the approach sound reasonable?

I introduced a new method called ViewExecutable::hasUrl() which does a similar check without throwing an exception, so there is nothing to catch. What do you think?

Hi @hussainweb, are you currently working further on this? I don't want to duplicate work again. :)

@xjm: Yes, I am. I am trying to figure out the exceptions thrown from renderMoreLink(). I will unassign myself at the end of the day.

@prashantgoel Thanks for helping with this critical. It is great to see everyone focusing on getting drupal 8 released.

To work on an issue that is assigned to someone, please get into irc in #drupal-contribute and say something to in the public channel using the person's irc name. This allows for coordination when work in moving fast and ongoing. If they are not in irc, try using their d.o contact form to communicate.
http://drupal.org/irc and https://www.drupal.org/irc/usage

I tracked down a few exceptions/failures to another issue with views and I have raised a critical bug at #2418163: Recent content view "more" link configuration is malformed. I don't think there is any need to postpone this as there are other unrelated failures that can be fixed. Particularly, failures in Drupal\node\Tests\NodeBlockFunctionalTest will be fixed when the other issue goes in.

I am calling it a day and unassigning as promised.

Ok, #2418163: Recent content view "more" link configuration is malformed is in. Should be unblocked now!

@webchick: Thanks! Doing a retest now.

Only 48 fails now, nice. ;)

Yes, it seems improved now.

Drupal\Tests\views\Unit\ViewExecutableTest::testGetUrlWithPlaceholdersAndArgs InvalidArgumentException: You cannot create a URL to a display without routes. /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/views/src/ViewExecutable.php:1743 /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/views/tests/src/Unit/ViewExecutableTest.php:173
This is something that is not allowing to create a route for test page in ViewExecutableTest.php.

@hussainweb, on last patch added by me seems I missed something. Apologies for the same.

@RavindraSingh: No need for apologies. We are all working to get this thing through. :)

I will resume later during the day, unless someone beats me to it. :)

Just a small step forward.

Fixed most test failures. Only the failures in BasicTest remain. Fixed none of the exceptions; they're all because the router doesn't know about the test view's routes, even though it should AFAICT. Still debugging.

Things I can't fix, should be fixed by others on this issue:

1. +++ b/core/modules/views/src/ViewExecutable.php
   @@ -1692,12 +1704,32 @@ public function buildTitle() {
   +  public function hasUrl($args = NULL, $display_id = NULL) {
   

   Missing docs.

2. +++ b/core/modules/views/src/ViewExecutable.php
   @@ -1721,41 +1759,41 @@ public function getUrl($args = NULL, $path = NULL) {
   +          // Gotta put something if there just isn't one.
   

   A more explanatory, less informal comment would be better here :)

3. +++ b/core/modules/views/tests/src/Unit/ViewExecutableFactoryTest.php
   similarity index 58%
   rename from core/modules/views/tests/src/Unit/ViewExecutableUnitTest.php
   
   rename from core/modules/views/tests/src/Unit/ViewExecutableUnitTest.php
   rename to core/modules/views/tests/src/Unit/ViewExecutableTest.php
   

   Good call.

Turns out those routes are missing because we're dynamically manipulating views in tests… without saving them. Simple solution: just save them after manipulating them. Except none of them can be saved, because all of them violate config schemas. IOW: it's incredible that the tests have been passing at all… :(

Currently fixing those violations; tedious job, so it will take some time, so assigning to myself to ensure nobody else does the same work.

Turns out those routes are missing because we're dynamically manipulating views in tests… without saving them. Simple solution: just save them after manipulating them. Except none of them can be saved, because all of them violate config schemas. IOW: it's incredible that the tests have been passing at all… :(

You know, I disagree. Your objects should work, as they are. Just imagine if the following code doesn't work:

$node = Node::create(['title' => 'bar']);
$node->label();

Just saying, those objects should work without their underlying storage, if possible.

This fixes all those exceptions. That leaves only the 2 failures in BasicTest. Hoping dawehner can fix those.

Just saying, those objects should work without their underlying storage, if possible.

Agreed in principle. But there is a problem with that, both in case of Views and Nodes/all other entities, we don't actually know if the test data is valid. And then we're not sure we're actually testing what the "real" entity contains/behaves like, we're testing a test version of it, that was valid at some point, but perhaps not anymore.

(This has caused problems elsewhere too.)

But, no matter, because in the above, I'm only saving those views that truly need to be saved.

It turns out that its good, that we have tests.

#63's interdiff was right, but was applied to #54 instead of #60. Rerolling.

Wow, it's green! Great work everyone. Did a quick scan:

1. +++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
   @@ -741,8 +741,22 @@ public function getPath() {
      /**
       * {@inheritdoc}
       */
   +  public function getRoutedDisplay() {
   
   +++ b/core/modules/views/src/Plugin/views/display/DisplayPluginInterface.php
   @@ -213,6 +213,8 @@ public function getLinkDisplay();
   +  public function getRoutedDisplay();
   +
      public function getUrl();
   
   +++ b/core/modules/views/src/ViewExecutable.php
   @@ -1692,12 +1704,32 @@ public function buildTitle() {
   +  public function hasUrl($args = NULL, $display_id = NULL) {
   

   Docs. :)

2. +++ b/core/modules/views/src/Plugin/views/display/DisplayRouterInterface.php
   @@ -46,4 +46,12 @@ public function alterRoutes(RouteCollection $collection);
      public function getUrlInfo();
   

   I believe @dawehner suggested we should remove this method -- on the other hand, since this is green now anyway, followup?

3. +++ b/core/modules/views/src/Tests/ViewExecutableTest.php
   @@ -319,18 +319,6 @@ public function testPropertyMethods() {
   -    // Test the getUrl method().
   -    $url = 'foo';
   -    $this->assertEqual($view->getUrl(NULL, $url), $url);
   -    // Test with arguments.
   -    $arg1 = 'bar';
   -    $arg2 = 12345;
   -    $this->assertEqual($view->getUrl(array($arg1, $arg2), $url), "$url/$arg1/$arg2");
   -    // Test the override_url property override.
   -    $override_url = 'baz';
   -    $view->override_url = $override_url;
   -    $this->assertEqual($view->getUrl(NULL, $url), $override_url);
   

   Note for reviewers: Updated test coverage is moved to the phpunit tests.

4. +++ b/core/modules/views/views.tokens.inc
   @@ -132,7 +133,7 @@ function views_tokens($type, $tokens, array $data = array(), array $options = ar
   -        $replacements += $token_service->generate('url', $url_tokens, array('path' => $path), $options);
   +        $replacements += $token_service->generate('url', $url_tokens, array('path' => $url->getInternalPath()), $options);
   

   Isn't getInternalPath() deprecated?

Thank you for the review! Working on it

Also see my review in #56.

Opened up a follow up: #2419121: Deprecate $display->getUrlInfo() and replace it with $display->getUrl()

Isn't getInternalPath() deprecated?

I still don't understand how token API works here, but it wants a path, so this is kinda the only partially official way to do it, IMHO.

A more explanatory, less informal comment would be better here :)

This is existing documentation :P

Reroll.

Let's fix that particular comment as well.

i'll fix the patch size.

Two super tiny nitpicks for when you reroll the patch to fix the patch size that can be fixed upon commit. RTBC when you've done that — I think I still can RTBC it since all I did was fix some test failures. I didn't do any work on the actual patch.

1. +++ b/core/modules/views/src/Plugin/views/display/DisplayPluginInterface.php
   @@ -213,8 +213,21 @@ public function getLinkDisplay();
   +   * If $this display has route information it is itself, otherwise it is the
   

   s/it is itself/it returns itself/

2. +++ b/core/modules/views/src/ViewExecutable.php
   @@ -1704,6 +1704,16 @@ public function buildTitle() {
   +   * Determines whether you can link to the view.
   

   s/view/View/
   ?

Let me quickly address that.

Polishing the docs a bit; will upload shortly.

1. +++ b/core/modules/views/src/Form/ViewsExposedForm.php
   @@ -115,7 +116,7 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   -    $form['#action'] = _url($view->display_handler->getUrl());
   +    $form['#action'] = $view->hasUrl() ? $view->getUrl()->toString() : Url::fromRoute('<front>')->toString();
   

   Hmm, is this the correct behavior actually? If I put an exposed form in a block, do I expect to be redirected to the front page, or the page I'm currently on? I'd say the latter.

2. +++ b/core/modules/views/src/Form/ViewsForm.php
   @@ -133,7 +133,7 @@ public function buildForm(array $form, FormStateInterface $form_state, ViewExecu
   -    $form['#action'] = $this->urlGenerator->generateFromPath($view->getUrl(), array('query' => $query));
   +    $form['#action'] = $view->getUrl()->setOption('query', $query)->toString();
   

   Why are we not checking hasUrl() here too?

Setting back to NR for clarification, at least.

+++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
@@ -741,8 +741,22 @@ public function getPath() {
+  /**
+   * {@inheritdoc}
+   */
   public function getUrl() {
-    return $this->view->getUrl();
+    return $this->view->getUrl(NULL, $this->display['id']);

+++ b/core/modules/views/src/Plugin/views/display/DisplayPluginInterface.php
@@ -213,6 +213,21 @@ public function getLinkDisplay();
+  /**
+   * Returns a URL to $this display or its configured linked display
+   *
+   * @return \Drupal\Core\Url|null
+   */
   public function getUrl();

Are we even using this anymore? It seems everywhere we're just calling straight through to the view. Edit: Lost some lines from the code snippet.

Doing a few more things actually.

lol w is close to x

+++ b/core/modules/views/src/ViewExecutable.php
@@ -1704,6 +1704,16 @@ public function buildTitle() {
+   * Determines whether you can link to the view.

s/view/View/
?

Hmm, I don't think so. "Views" is the name of the module, so capitalized. "A view" is just a thing. We don't say "a Term" or "a Node" so I think it's just "a view". OTOH we are probably totally inconsistent either way. ;)

+++ b/core/modules/views/src/ViewExecutable.php
@@ -239,9 +241,9 @@ class ViewExecutable {
   /**
    * Allow to override the url of the current view.
    *
-   * @var string
+   * @var \Drupal\Core\Url
    */
...
+  public $override_url;

@@ -1693,11 +1705,41 @@ public function buildTitle() {
+    if (!empty($this->override_url)) {
+      return TRUE;
+    }
...
     if (!empty($this->override_url)) {
       return $this->override_url;
     }

+++ b/core/modules/views/tests/src/Unit/ViewExecutableTest.php
@@ -73,6 +111,140 @@ protected function setUp() {
+  public function testGetUrlWithOverriddenUrl() {
+    $url = Url::fromRoute('example');
+    $this->executable->override_url = $url;
+
+    $this->assertSame($url, $this->executable->getUrl());
+  }

This public variable is documented, but never written in core outside tests. What's the usecase? Do we still need it?

Sorry for all the little comments; keep finding new questions as I work through the patch.

+++ b/core/modules/views/src/ViewExecutable.php
@@ -1693,11 +1705,41 @@ public function buildTitle() {
+   * Determines whether you can link to the View.
+   *
+   * @param array $args
+   *   (optional) The arguments.
+   * @param string $display_id
+   *   (optional) The display ID.
+   *
+   * @return bool
+   */
+  public function hasUrl($args = NULL, $display_id = NULL) {
+    if (!empty($this->override_url)) {
+      return TRUE;
+    }
+
+    $display_handler = $this->displayHandlers->get($display_id ?: $this->current_display)->getRoutedDisplay();
+    if (!$display_handler instanceof DisplayRouterInterface) {
+      return FALSE;
+    }
+
+    return TRUE;
+  }

Should the fallback behavior here really be to return true? I'd think if a view has no displays with valid routes and no other way of setting the URL, it's not linkable. I guess we could add a test.

1. +++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
   @@ -741,8 +741,22 @@ public function getPath() {
   +  public function getRoutedDisplay() {
   +    if ($this instanceof DisplayRouterInterface) {
   +      return $this;
   +    }
   +
   +    $display_id = $this->getLinkDisplay();
   +    if ($display_id && $this->view->displayHandlers->has($display_id) && is_object($this->view->displayHandlers->get($display_id))) {
   +      return $this->view->displayHandlers->get($display_id)->getRoutedDisplay();
   +    }
   +  }
   

   So there's a recursive behavior here. What happens if you have two displays that somehow reference each other as the linked display? ;)

2. +++ b/core/modules/views/src/ViewExecutable.php
   @@ -1693,11 +1705,41 @@ public function buildTitle() {
   +    $display_handler = $this->displayHandlers->get($display_id ?: $this->current_display)->getRoutedDisplay();
   +    if (!$display_handler instanceof DisplayRouterInterface) {
   +      return FALSE;
   +    }
   

   In the current behavior of getRoutedDisplay(), I don't think a non-routed display can ever be returned, so the instanceof check might be redundant.

Meanwhile, some docs and a minor refactoring for self-documentingness of the code. Doesn't include any changes for #78 and beyond.

....duh

Some day I will learn to listen to the little red squiggles in PHPStorm.

+++ b/core/modules/views/src/Plugin/views/display/PathPluginBase.php
@@ -502,8 +500,16 @@ public function getRouteName() {
+   * {@inheritdoc

Missing closing curly.

1. +++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
   @@ -742,10 +742,13 @@ public function getPath() {
   +    // If this display has a route, return this display.
        if ($this instanceof DisplayRouterInterface) {
          return $this;
        }
    
   +    // If the display does not have a route (e.g. a block display), get the
   +    // route for the linked display.
        $display_id = $this->getLinkDisplay();
   

   +2 to document this!

2. +++ b/core/modules/views/src/Plugin/views/display/DisplayRouterInterface.php
   @@ -49,9 +49,26 @@ public function getUrlInfo();
   +   * The default route name for a display is views.view_id.display_id. Some
   ...
   +   *   view_id.display_id and the values are the route names.
   

   Should we use $view_id.$display_id here? Maybe this explains it a bit better.

3. +++ b/core/modules/views/src/Plugin/views/display/DisplayRouterInterface.php
   @@ -49,9 +49,26 @@ public function getUrlInfo();
   +  public function getAlteredRouteNames();
   
   sky interdiff reviewing! Sorry, but I'm really flashed by this feeling.
   
   +++ b/core/modules/views/src/Plugin/views/display/PathPluginBase.php
   @@ -502,8 +500,16 @@ public function getRouteName() {
   +  /**
   +   * {@inheritdoc
   +   */
   +  public function getAlteredRouteNames() {
   +    return $this->state->get('views.view_route_names') ?: array();
   +  }
   +
   

   Does this method has to be public? I just see on usecase of it. +1 though for doing it.

+++ b/core/modules/views/src/Plugin/views/display/PathPluginBase.php
@@ -307,7 +307,7 @@ public function getMenuLinks() {
-          'route_name' => $this->getRouteName();
+          'route_name' => $this->getRouteName(),

sad PHP.

Discussed with @webchick, @effulgentsia, @alexpott, and @catch. This issue has been triaged as critical by the maintainers because it is a hard blocker to #2343669: Remove _l() and _url(), which in turn is necessary before release because the legacy API does not properly support the D8 link APIs and routing system.

@dawehner, For 2nd. I think this is fine to use view_id.display_id
* view_id.display_id and the values are the route names.

@RavindraSingh
Well, there is fine and there is making it easy for people to understand what is going on.
@xjm Do you have time to fix those bits?

Tried to answer a couple of those good questions.

Hmm, is this the correct behavior actually? If I put an exposed form in a block, do I expect to be redirected to the front page, or the page I'm currently on? I'd say the latter.

If you look at the current code in HEAD, the flow of code would return an empty string ... which afaik returns $basepath basically.

Why are we not checking hasUrl() here too?

I guess we should actually move this logic into View::getUrl(), given that this kinda worked before as well.

Are we even using this anymore? It seems everywhere we're just calling straight through to the view. Edit: Lost some lines from the code snippet.

Well, even if we don't do anymore, I think its good to have a method on the display, given that a view is just a collection of display, which each has an URL or not.

Hmm, I don't think so. "Views" is the name of the module, so capitalized. "A view" is just a thing. We don't say "a Term" or "a Node" so I think it's just "a view". OTOH we are probably totally inconsistent either way. ;)

Given that you are in the domain of views already here, "a view" should not be confusing, much like "a node" is also not confusing for people.

This public variable is documented, but never written in core outside tests. What's the usecase? Do we still need it?

Ctools in D7 used override_path itself, but at the same time I think override_urlcould be helpful to place for example an exposed form at some random place.

Should the fallback behavior here really be to return true? I'd think if a view has no displays with valid routes and no other way of setting the URL, it's not linkable. I guess we could add a test.

It would be always nice to have a test, what kind of question is that ;)

So there's a recursive behavior here. What happens if you have two displays that somehow reference each other as the linked display? ;)

You pretty much end up with just displays with have a DisplayRouterInterface, see:

        foreach ($this->view->storage->get('display') as $display_id => $display) {
          if ($this->view->displayHandlers->get($display_id)->hasPath()) {
            $options[$display_id] = $display['display_title'];
          }
        }

In the current behavior of getRoutedDisplay(), I don't think a non-routed display can ever be returned, so the instanceof check might be redundant.

Well, getRoutedDisplay() also allows to return NULL, don't we want to ensure that we check it? Note: The documentation of that method should tell that
it returns DisplayRouterInterface instead.

@mpdonadio++

One more nitpick.

+++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
@@ -726,8 +726,28 @@ public function getPath() {
+    // No routed display exists, so return null
+    return null;

We use NULL IMHO.

Thank you!

Awesome. Getting this one in while it's hot!

Committed and pushed to 8.0.x. Thanks!

WOOOOO

mpdonadio+++++++++++