Use route system instead of hook_menu() in Views

Before someone is working on that, i do think we should wait until it's clear which route we are going in core.

Re-postponing. :)

Assign to myself to figure out how far we can get at the moment.

hey

This patch allows you to use a view with the router and even all kind of arguments.
This does not remove any big chunks of code yet, due to sanity to not remember specific support for specific stuff.

just posting the newest patch.

boom

Let's see how many tests fail with that version.

Removed rebuilding of the router, so let's see how much fails with that.

Just runned the first view via a subrequest and it worked fine!

Wrong patch.

A couple of workarounds later.

Let's see how many test failures are left, this is feeling better.

Let's upload the first green patch.

Maybe that one.

He, failed on an actual new test added by that patch :)

Do you think we should give this a new name? like uses_route or something?

Well, we need one which fits for both hook_menu and uses_route, as you will for now require both (but not for feed).
I'm wondering whether we should do all of that in this patch or clean it up later.

Seems like the logic from collectRoutes (when there is some) should be on the RouteSubscriber instead?
I'm not sure, as especially the display plugin have to be able to define the routes, as the logic is on there, and probably should stay there.
I agree that potentially this collectRoutes on the ViewExecutable should be skipped and the display would be used directly.

So I guess we keep this for tabs etc...? So maybe at some point we can change this to executeMenuLocalTasks or something.

We have to, as menu links is pretty important :) We have to maybe add executeMenuLocalTasks and keep executeHookMenu for the normal links, but that's all not sure yet.

Could we just replace this with $this->enableModules(array('system'))? Then it should install those schemas anyway I think.

So enable system would cause quite a lot of new modules, so I would really suggest to stay away from that, as three items is way faster.

Instead of doing this, couldn't we just store the views args in a views_args array in attributes? Not sure.

Well, as far as I know you can only map url values to attributes in the request, but not to a subarray.

#26: drupal-1800998-26.patch queued for re-testing.

Ups.

+++ b/core/modules/views/lib/Drupal/views/EventSubscriber/RouteSubscriber.php
@@ -0,0 +1,45 @@
+    $views = views_get_applicable_views('uses_route');

I'm assuming this can get refactored to an object later?

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/DisplayPluginBase.php
@@ -2408,6 +2409,17 @@ public function renderMoreLink() {
   /**
+   * Adds the route entry of a view to the collection.#
+   *
+   * @param \Symfony\Component\Routing\RouteCollection $collection
+   *   A collection of routes that should be registered for this resource.
+   */
+  public function collectRoutes(RouteCollection $collection) {
+

This is usually a sign of a design flaw. Why not make a RoutableViewInterface with a collectRoutes() method. Then a display plugin can implement that interface and method or not. If not, then it has no need for this method at all.

The subscriber above can then simply wrap an instanceof check around the method call, which would avoid having or calling null methods like this.

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/Page.php
@@ -92,7 +93,10 @@ public function execute() {
-    return $render;
+    $response = $this->view->getResponse();
+    $response->setContent(drupal_render_page($render));
+
+    return $response;

Why is this necessary? This should be handled by the view subscribers later on. This is on a Page display, which, assuming that survives B&L, should be responsible only for the body area of the page.

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/PathPluginBase.php
@@ -33,6 +35,57 @@ protected function defineOptions() {
+    // @todo How do we apply argument validation?
+    $bits = explode('/', $this->getOption('path'));
+    // @todo Figure out validation/argument loading.
+    // Replace % with %views_arg for menu autoloading and add to the
+    // page arguments so the argument actually comes through.
+    $arg_counter = 0;

I think we may want to consider a pluggable method similar to access control for extended validation. The _requirements array works for simple values (regexes and single-key values), but we may need more complex rules. Or maybe we can get away with just setting _requirements like we do for access, and having extended validators? They'd return a 404 instead of a 403 the way access does.

That may be best as a spinoff issue if we can justify it as not a "new feature".

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/PathPluginBase.php
@@ -33,6 +35,57 @@ protected function defineOptions() {
+    $route = new Route($route_path, array(
+      '_controller' => 'views.page_controller:handle',
+      'view_id' => $view_id,
+      'display_id' => $display_id,
+    ));
+

I think this should be _content, not _controller, so that the normal Drupal controller can take over. Assuming we're dealing with a page display, not a feed, which should be handled differently. (A Page display uses the Drupal controller, whatever it is; a feed has its own controller, since it is responsible for the entire response.)

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/PathPluginBase.php
@@ -33,6 +35,57 @@ protected function defineOptions() {
+    if (is_array($callback)) {
+      list($access_callback, $args) = $callback;
+      $route->addOptions(array(
+        '_access_callback' => $access_callback,
+        '_access_arguments' => $args,

Oh this is not good at all. We are trying to eliminate functions as access callbacks since they cannot be lazy loaded.

Since Views access control is a plugin, it should be able to simply have a _my_plugin: TRUE flag, like the existing access checkers, and then we call it like any other access checker. That would be much cleaner than bridging to functions.

+++ b/core/modules/views/lib/Drupal/views/Tests/ViewPageControllerTest.php
@@ -0,0 +1,81 @@
+    try {
+      $this->pageController->handle($request);
+    }
+
+    catch (\Exception $e) {
+      $exception_found = TRUE;
+    }
+    $this->assertTrue($exception_found, 'Exception thrown when view was not found');

I think the preferred way to structure this sort of check is:

try {
  thing_that_fails();
  $this->fail('No exception. :-(');
}
catch (ExpectedException $e) {
  $this->pass('Yay, it broke!');
}

+++ b/core/modules/views/lib/Drupal/views/ViewPageController.php
@@ -0,0 +1,57 @@
+    $entities = $this->entityManager->getStorageController('view')->load(array($view_id));

We should be able to do this natively in the router now. No need to do so here.

+++ b/core/modules/views/lib/Drupal/views/ViewPageController.php
@@ -0,0 +1,57 @@
+    $args = array();
+    foreach ($request->attributes->all() as $key => $argument) {
+      if (strpos($key, 'arg') !== FALSE) {
+        $args[] = $argument;
+      }

I would love to do something cleaner than this. Don't arguments have to have a machine name in the View config? Can't we use those? Or am I confusing them with normal filters?

+++ b/core/modules/views/lib/Drupal/views/ViewsAccessCheck.php
@@ -0,0 +1,40 @@
+class ViewsAccessCheck implements AccessCheckInterface {

See above for why I think this checker is a bad idea. Much of the work that's gone into Drupal 8 is exactly so that modules like Views don't need to build their own systems on top of core that core should already be doing. It's doing it now, so let's use it directly. :-)

+++ b/core/modules/views/views.module
@@ -695,6 +695,12 @@ function views_invalidate_cache() {
+  // Set the router to be rebuild.
+  // @todo This isn't a good fix.
+  if (db_table_exists('router')) {
+    drupal_container()->get('router.builder')->rebuild();

Ack. No it's not. Why do you need this table check here? Avoiding rebuilding before the router is initialized shouldn't be your problem. If the builder needs that check internally we can add it, although I suspect that's a sign of some other problem.

Ah someone providing feedback that's good, thanks!

Let's talk about the access part in IRC, but here a bit of explanation why I went this route:
Currently a view can be embedded into a page/block/where-else and all of these places use the same kind of access check via the access plugins. If you would just use access subscribers, it seems to be impossible to use access on other levels like blocks?
I guess on the longrun we could use access checks on subroutes and just provide configuration via the views UI?

I'm assuming this can get refactored to an object later?

Good idea: Here is a follow up for that: #1918860: Move views_get_applicable_views into a class

This is usually a sign of a design flaw. Why not make a RoutableViewInterface with a collectRoutes() method. Then a display plugin can implement that interface and method or not. If not, then it has no need for this method at all.

Yeah I'm not yet into interfaces that much. In general I'm wondering whether we should keep our "uses_route" on the plugin definition, as it's exactly used for that: Determine which plugins have which feature. Do you agree with dropping the uses_route key, and allow to fetch views_get_applicable_view somehow the information from interfaces as well. Do you have an oppinion whether RouteDisplayInterface makes more sense, as it's here we talk about displays, not about view/viewExecutables.

Why is this necessary? This should be handled by the view subscribers later on. This is on a Page display, which, assuming that survives B&L, should be responsible only for the body area of the page.

At the moment lower levels, then the displays have to be able to change the response. For example the rss style, wants to set it to application/rss+xml, or some kind of handler might want to set the response code to any specified value. (template_preprocess_views_view_rss() for reference, even this is certainly not the best place to do it).

I think we may want to consider a pluggable method similar to access control for extended validation. The _requirements array works for simple values (regexes and single-key values), but we may need more complex rules. Or maybe we can get away with just setting _requirements like we do for access, and having extended validators? They'd return a 404 instead of a 403 the way access does.

Yeah all more like pager manager related stuff need that. Should I create a WSCCI issue for that?

I think this should be _content, not _controller, so that the normal Drupal controller can take over. Assuming we're dealing with a page display, not a feed, which should be handled differently. (A Page display uses the Drupal controller, whatever it is; a feed has its own controller, since it is responsible for the entire response.)

See my reason for that above, but I'm 100% open for cleaner solutions.

Oh this is not good at all. We are trying to eliminate functions as access callbacks since they cannot be lazy loaded.

Can you specify \Drupal\foo\Bar\Baz::foo() and it will be loaded if available, when you call cuf(a)?

Since Views access control is a plugin, it should be able to simply have a _my_plugin: TRUE flag, like the existing access checkers, and then we call it like any other access checker. That would be much cleaner than bridging to functions.

Just to summarize: Every access plugin should provide it's own route access subscriber (if needed) instead of using callbacks? This seems to be a great idea for me, will refactor it later.

We should be able to do this natively in the router now. No need to do so here.

I tried that before, but $variable_names = $route->compile()->getVariables();seems to be empty because the view ID is not part of the URL.

I would love to do something cleaner than this. Don't arguments have to have a machine name in the View config? Can't we use those? Or am I confusing them with normal filters?

They absolutely do.

Additional I have been thinking of the place of argument default plugins in this new world, but again, I got stopped because they are still not only part of the route.

Ack. No it's not. Why do you need this table check here? Avoiding rebuilding before the router is initialized shouldn't be your problem. If the builder needs that check internally we can add it, although I suspect that's a sign of some other problem.

Some of the tests complained about that, but I didn't went into more research: If this cache clear would be part of the container, we could inject the router builder, so we would be sure that the table exists all the time, as the builder knows about it?

And the interdiff, just for reference.

Re interfaces: The one drawback of an interface as the "flag" is that it's only useful at runtime. You have to have the class parsed into memory at least in order to do class_implements(), or have an instantiated object to do instanceof. So for the code I highlighted above an interface is all you need. If you need a "get me all plugin types that will use routes" operation, that can be done with interfaces but gets very expensive.

(That said, if we can do static code analysis, like the registry used to and the annotation support does now, that could be a good way around it. That would be awesome but is not something we can do in this issue. So, maybe do both for now and see if we can rip out the flag later.)

I am not convinced that it's wise to allow any handler/style to change things like the mime type. If you're returning RSS, that's something the display plugin knows and should control, and that's the end of it.

call_user_func_array('\My\Class\Here', 'aMethod'); will call aMethod statically on \My\Class\Here. That's only marginally better than a function. call_user_func_array($an_object, 'aMethod'); will call aMethod on that object. The Routing system has two alternate syntaxes inherited from Symfony that the ControllerResolver takes care of: \My\Class::aMethod instantiates \My\Class first and then calls aMethod, while some_service:aMethod retrieves some_service from the DIC and then calls aMethod. You'd have to reimplement that here if you wanted to use those, though.

The preferred approach is indeed an access checker service; the system is designed so you could write 30 of them, but only the ones that care about a given route will be instantiated when needed. And "When needed" is up to the checker to decide based on anything it wants on the route definition.

The view is in the attributes array, and is therefore available to the ParamConverter system, which already handles any entity whose parameter name matches its entity machine name. If you need more, write a new ParamConverter.

I am not convinced that it's wise to allow any handler/style to change things like the mime type. If you're returning RSS, that's something the display plugin knows and should control, and that's the end of it.

At the moment this is not really the case. Let's take the RSS example, as it's a good one. The display would be not an RSS display but an abstract feed display. This allows you to write for example an atom style plugin without having to worry about the display, even you could still set the http headers. Happy enough you actually wrote that one.
Another example of that, which is indeed already in core is the REST_Export display that allows any kind of output. Sure you could ask the style/row/whatever plugins for explicit mime types changes as well, but who can know what a potential contrib module want to change.

Summary from IRC conversation with @dawehner:

1) Between the access checker system, custom request listeners, and the fact that a controller can still throw an AccessDenied/NotFound exception if it wants, we're not going to add a dedicated validation system at this time. There's enough places already that you can do such things that we're going to punt on it. If it turns out we need a common system contrib can add it easily, and we can revisit moving it into core in Drupal 9.

2) Views access control will be 2-part: A Views Access Plugin will be responsible ONLY for annotating a route definition with appropriate data. Actually enforcing that access control will be handled by a normal Access Checker as it already exists today.

For instance, for the Role-based access control, the Views Access Plugin will only be responsible for adding requirements: _role: "do something cool" to the route definition. A completely Views-agnostic (even if it may just happen to live in the Views module) Access Checker will then enforce that, exactly as the PermissionAccessChecker does now. Then you can specify _role yourself in a route definition, or Views can, or some other module can, or whatever. Actual enforcement is entirely decoupled, which is good.

3) Non-Page Displays (eg, Feeds) will be wired to the route directly as a _controller. There's no need for an extra layer there. Style plugins may know what mime type their return data is supposed to be, but will *not* have a Response object themselves. That is, the Feed display will call getContent() on the Style plugin, which will return a string, and getMimeType() on the Style plugin, which returns another string. (Or something along those lines.) The Feed will then use that data to assemble a Response object. The Style plugin and lower will have no dependency on HttpFoundation or the concept of HTTP.

just tracking work.

Added a full test for role based accessChecker.

This could fix a tone of the problems

Let's see.

Dear Drupal Kernel, please just register by test bundles!

#44: drupal-1800998-44.patch queued for re-testing.

Let's get rid of the installation problem and see which tests are still broken.

#48: drupal-1800998-48.patch queued for re-testing.

Wow, days of debugging for such a trivial change.

This patch might include more changes, but I just post the important difference.

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/PathPluginBase.php
@@ -33,6 +35,61 @@ protected function defineOptions() {
+      $access_plugin = drupal_container()->get('plugin.manager.views.access')->createInstance('none');
+    }

This should probably switch over to Drupal:: now that we have that.

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/PathPluginBase.php
@@ -59,49 +116,15 @@ public function executeHookMenu($callbacks) {
+        // @todo _menu_router_build() denies access to paths without a page
+        //   callback.
+        'page callback' => 'NOT_USED',
+        'page arguments' => array(),
         // Default access check (per display).
-        'access callback' => 'views_access',
-        'access arguments' => $access_arguments,
+        'access callback' => TRUE,
         // Identify URL embedded arguments and correlate them to a handler.

As of last night, page callback and access callback can be left out entirely. Just specify "route" and the machine name of the route and the rest will be handled automatically.

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/PathPluginBase.php
@@ -158,8 +181,10 @@ public function executeHookMenu($callbacks) {
-              'page callback' => 'views_page',
-              'page arguments' => $page_arguments,
+              // @todo _menu_router_build() denies access to paths without a page
+              //   callback.
+              'page callback' => 'NOT_USED',

Ibid.

+++ b/core/modules/views/lib/Drupal/views/ViewsAccessCheck.php
@@ -0,0 +1,37 @@
+/**
+ * Defines a route access checker for the _access_all_views permission.
+ *
+ * @todo We could leverage the permission one as well?
+ */
+class ViewsAccessCheck implements AccessCheckInterface {

I don't get why we have this, when it's just a permission check. Why not just use the existing permission checker?

+++ b/core/modules/views/tests/views_test_data/lib/Drupal/views_test_data/Access/DynamicTestAccessCheck.php
@@ -0,0 +1,55 @@
+/**
+ * Defines a access checker for views tests.
+ */
+class DynamicTestAccessCheck implements AccessCheckInterface {
+

I know this is in a test module, but... huh?

Also, we now have _content and _form default keys in routes. I wonder... should we add a _views key that we can then enhance to the appropriate View controller, just like we do with pages and forms? Rather than hard-coding the controller into the route definition? I'm not sure if that makes sense here, but it's worth asking.

Also, we now have _content and _form default keys in routes. I wonder... should we add a _views key that we can then enhance to the appropriate View controller, just like we do with pages and forms? Rather than hard-coding the controller into the route definition? I'm not sure if that makes sense here, but it's worth asking.

I see your point, as we have multiple router items using the same controller. One problem is thought that it feels like a hack.
_form and _content are used both for wrappers, though a view controller does seems to be anything that is needed already, or not? Maybe we should think about something like that on a follow up?

I know this is in a test module, but... huh?

Well, yeah I struggled with that, but I also didn't wanted to remove test coverage for that usecase. There should be certainly the test which uses $view->access() directly, but I see that it's quite complicated for just a "simple" test.

Blocked on #1938960: _menu_translate() doesn't care about new routes currently.

#55: drupal-1800998-54.patch queued for re-testing.

Rerolled the patch.

Forgot some files to add.

Add tag.

#61: drupal-1800998-61.patch queued for re-testing.

Fixed one issue in the node rss test, which tests for rss.xml/foo/bar, which simply returns 403 in the world of route items.
The current problem is that the AccessTest for the dynamic access plugin doesn't even consider to execute the DynamicTestAccessCheck when called with the "test_access_dynamic/foo/bar" path, @see AccessTest::testDynamicAccessPlugin() and DynamicTestAccessCheck::access().
This access method though is called for test_access_dynamic, that's why i'm really confused.

I always get confused when I see this in my list :)

+++ b/core/modules/user/lib/Drupal/user/Access/RoleAccessCheck.php
@@ -0,0 +1,47 @@
+ * Determines access to routes based on roles defined via hook_permission().

Roles are not defined by hook_permission. They're defined by a user.

+++ b/core/modules/user/lib/Drupal/user/Access/RoleAccessCheck.php
@@ -0,0 +1,47 @@
+  public function applies(Route $route) {
+    return array_key_exists('_role_id', $route->getRequirements());
+  }

We should probably make this _role, not _role_id. We're using _permission, even though what's being specified is the permission machine name rather than the human-readable name.

+++ b/core/modules/user/lib/Drupal/user/Access/RoleAccessCheck.php
@@ -0,0 +1,47 @@
+    $diff = array_diff(array_filter($rids), $roles);
+    if (empty($diff)) {
+      return TRUE;
+    }
+    else {
+      return NULL;

Hm. I don't believe any other access checkers do this comma-separated list thing. I don't know if that's good or bad. :-)

I suppose since Views does allow "OR" for role and permission, it is necessary. In that case, we should probably do the same for _permission checks to be consistent.

+++ b/core/modules/user/lib/Drupal/user/Plugin/views/access/Permission.php
@@ -30,11 +31,14 @@ class Permission extends AccessPluginBase {
-    return views_check_perm($this->options['perm'], $account);
+    return user_access($this->options['perm'], $account) || user_access('access all views', $account);

Because then we can more easily mirror this check (which I assume has to be here for non-route views?) like so:

_permission: "something, access all views"

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/PathPluginBase.php
@@ -159,11 +177,8 @@ public function executeHookMenu($callbacks) {
+              // @todo _menu_router_build() denies access to paths without a page
+              //   callback.

This @todo can now go away in favor of adding a route_name. Though I don't fully understand what this code is doing as it looks like a duplicate of the previous block in the patch, which says it's the same method, so I'm totally confused. :-)

+++ b/core/modules/views/lib/Drupal/views/Tests/Plugin/AccessTest.php
@@ -49,7 +54,7 @@ protected function setUp() {
-  function testAccessNone() {
+  function _testAccessNone() {

I have no idea why you're putting a _ prefix on this method...

But all methods should have a visibility declared.

+++ b/core/modules/views/lib/Drupal/views/Tests/Plugin/DisplayPageTest.php
@@ -34,18 +53,56 @@ public static function getInfo() {
+    $subrequest = Request::create('/test_page_display_403', 'GET');
+    $response = $this->container->get('http_kernel')->handle($subrequest, HttpKernelInterface::SUB_REQUEST);
+    $this->assertEqual($response->getStatusCode(), 403);

The idea that we can do this rather than drupalGet() makes me warm and fuzzy inside. :-)

+++ b/core/modules/views/views.module
@@ -687,6 +676,12 @@ function views_invalidate_cache() {
+  // Set the router to be rebuild.
+  // @todo This isn't a good fix.
+  if (db_table_exists('router')) {
+    drupal_container()->get('router.builder')->rebuild();
+  }

This should be Drupal::getService('router.builder') now.

What I don't get about #66 is why DynamicAccesCheck is here. It appears to be just for the test module, so... what's it testing? If that's a problem, why not eliminate the problem? I still don't get it. :-)

The role access checker could arguably be spun off to its own issue if you want, to keep the patch size down.

The breadcrumb tests are worth retesting in light of recent patches that went in just in the last day or so from Tim. Will retest in a moment. I don't know why testbot is not showing me what the actual failures were on the other tests...

Thanks for another great review! Sorry for sticking some debugging changes in there.

Yeah the plan was always to split up the role changes, so here we go: #1956896: Add role based access checker

Btw. you convinced me to remove the DynamicAccessTest as it was mostly about checking some code in hook_menu which is removed with that patch, so this would just test whether the route system would be able to support dynamic access arguments, which for sure can be done, if you use your own access checker.

Tim wanted to look at the breadcrumb tests, which is great!

I will fix your points once the role issue is in?

#66: drupal-1800998-66.patch queued for re-testing.

This was mentioned during our REST call. We think this would help us keep Views' serialization working as we transition to HAL from JSON-LD. Thanks!

#1956896: Add role based access checker was just committed. Shall we get this moving again?

Yes, i will care about this on the weekend.

Because then we can more easily mirror this check (which I assume has to be here for non-route views?) like so:

Yeah exactly. Well on the longrun you might be able to just replace everything by subrequests. Just for example an embedded view in some kind of template
is simply just that.

I suppose since Views does allow "OR" for role and permission, it is necessary. In that case, we should probably do the same for _permission checks to be consistent.

That's why there is #1984528: Follow-up: Allow for more robust access checking

This @todo can now go away in favor of adding a route_name. Though I don't fully understand what this code is doing as it looks like a duplicate of the previous block in the patch, which says it's the same method, so I'm totally confused. :-)

Oh right, that's what we alread added in the patch.

I have no idea why you're putting a _ prefix on this method...

But all methods should have a visibility declared.

That's just my own way to make tests run faster, i'm sorry.

I also removed all this dynamic test bits.

I know the breadcrumb tests still fail, but this seems to be just a small detail.

The javascript failure seemed to be just random.

The failure in the breadcrumb bit is caused by the fact that even the menu_link table has the route_name, it's not stored when you add a new menu_link via the UI/API, unless you specify the route_name.

This patch adds a small helper function which fetches the route_name from the menu_route table and stores it on the menu_link.

+++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkStorageController.php
@@ -216,6 +216,34 @@ protected function preSave(EntityInterface $entity) {
+    // Not during a menu rebuild, so look up in the database.
+    $route_name = (string) db_select('menu_router')
+      ->fields('menu_router', array('route_name'))
+      ->condition('path', $ancestors, 'IN')
+      ->orderBy('fit', 'DESC')
+      ->range(0, 1)
+      ->execute()->fetchField();
+

The menu_router table is hopefully going away. What exactly are you trying to do here? Remember that path does not map 1:1 to route anymore.

Hitting the DB directly is almost always wrong at this point. :-) (Even then, you should be injecting the connection.)

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/Page.php
@@ -92,7 +93,10 @@ public function execute() {
-    return $render;
+    $response = $this->view->getResponse();
+    $response->setContent(drupal_render_page($render));
+
+    return $response;

This is still wrong, I think. Although what right is depends on what happens with Scotch. :-/

Thanks for the review!

There is another issue to allow inject dependencies into entity controllers: #1909418: Allow Entity Controllers to have their dependencies injected

This is still wrong, I think. Although what right is depends on what happens with Scotch. :-/

Can we agree to get this in like this, as it seems to be the way to do it at the moment (without scotch).

The menu_router table is hopefully going away. What exactly are you trying to do here? Remember that path does not map 1:1 to route anymore.

Hitting the DB directly is almost always wrong at this point. :-) (Even then, you should be injecting the connection.)

Yeah you are totally right. We can ask the route system to give us the matching route.

Can we agree to get this in like this, as it seems to be the way to do it at the moment (without scotch).

Yeah, I'm not sure what the right way is there, so until then "it works" is good enough, I suppose.

let's also catch some exceptions so the installation works again.

#85: drupal-1800998-85.patch queued for re-testing.

Every single upgrade path test is hitting a 30 second max execution time.
No idea why :(

I have seen cases before where an infinite loop that involves a menu rebulid will hit the 30 second timeout before hitting the 100 function stack limit. That makes it look like a timeout when it's really an infinite loop problem. Perhaps that's the case here?

Yes, the 100 function recursion limit is a xdebug feature, without that exception, a recursion goes on until it hits the max execution time.

Yeah there is a recursion involved.

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/DisplayRouterInterface.phpundefined
@@ -0,0 +1,28 @@
+ * Additional to implement the interface you should still put "uses_routes" into

Additionally? Also i think this should be rephrased to:

Additionaly to implementing the interface "uses_routes" should be used into
the plugin definition.

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/DisplayRouterInterface.phpundefined
@@ -0,0 +1,28 @@
+   * Adds the route entry of a view to the collection.#

i guess # is not needed

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/PathPluginBase.phpundefined
@@ -34,6 +37,61 @@ protected function defineOptions() {
+   * Implements \Drupal\views\Plugin\views\display\DisplayRouterInterface::collectRoutes().

inheritdoc

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/PathPluginBase.phpundefined
@@ -34,6 +37,61 @@ protected function defineOptions() {
+    // @todo Figure out validation/argument loading.
...
+    foreach ($bits as $pos => $bit) {
+      if ($bit == '%') {
+        $arg_id = 'arg_' . $argument_ids[$arg_counter++];
+        $defaults[$arg_id] = '';
+        $bits[$pos] = '{' . $arg_id . '}';
+      }
...
+    // Add missing arguments not defined in the page settings.
+    while (($total_arguments - $arg_counter) > 0) {
+      $arg_id = 'arg_' . $argument_ids[$arg_counter++];
+      $bit = '{' . $arg_id . '}';
+      $defaults[$arg_id] = '';
+      $bits[] = $bit;
+    }

this took me a while to figure out..even if there is a @todo, maybe transfer it to a seperate protected method?

+++ b/core/modules/views/views.moduleundefined
@@ -690,6 +679,13 @@ function views_invalidate_cache() {
+  // Set the router to be rebuild.
+  // @todo Figure out why the cache rebuild is trigged but the route table
+  //   does not exist yet.
+  if (db_table_exists('router')) {
+    Drupal::service('router.builder')->rebuild();
+  }

is this still a case?

Try: "In addition to implementing the interface, specify 'uses_routes' in the plugin definition."

is this still a case?

Yes, but maybe that's another issue of the bootstrap like on the other issue.

If we are checking this here, we should change the method signature to ($account = NULL)

The only place which calls this code is in DisplayPluginBase::access() which passes in the account, so there is no need to check.

This is an associative array of rid => rid now, so do we need to do this? array_intersect would still work fine.

Good point.

If a user didn't have a permission wouldn't this ternary evaluate to FALSE, so NULL would be used? So instead of failing access we are passing it on?

No it should be NULL, because FALSE means in the new access manager system that the user will not have access at all to the route. NULL means (let's other plugins decide, so for example the role based one can tell it's opinion).

+++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkStorageController.php
@@ -224,7 +224,7 @@ protected function preSave(EntityInterface $entity) {
+   * Returns the route_name matching an URL.

"a URL", not "an". :-)

Other than that... I think we're finally done here.

"a URL", not "an". :-)

I have to trust you :) The relation between "an URL" and "a URL" in core is 43:2.

This time with the actual changed string.

Comment #100 FTW!

Extreme nitpick .. :)

We found a pretty serious bug testing this patch (actually several, one in the patch and one outside it). I'll re-confirm and then post details.

Yeah, unfortunately, this is majorly broken. At least some contextual filters do not work with this patch.

STR:

1. Install Standard.
2. Create two nodes: one article, and one page, both promoted to the front page.
3. Edit the /node frontpage view to have the path front (to avoid a route conflict with node/{node} in the node module; do we want to consider changing the path for /node? Followup?).
4. Add a contextual filter to the view for the content type.
   
5. Use the default configuration for the contextual filter, except change "When the filter value is not in the URL" to "Page not found".
   
6. Visit /front. You should properly get a 404.
7. Visit /front/page. In HEAD, you will correctly see your page node. With this patch applied, you get a 404.

Also, this issue feels like at least a major to me. Could we even ship without doing this or severely compromising WSCCI's goals?

Note that I also got a similar result with roles, but roles are also broken in HEAD, which I'll file separately and link here.

#1995868: Fatal when using a role contextual filter is the other issue we discovered in testing.

Thanks for testing this patch!

Changed a couple of things here:

• Renamed the internal route names from $view_id.$display_id to include a "view." prefix.
• Fixed how routes deal with optional/required arguments. (there is new test coverage for that)
• Fixed the actual problem spotted on #103
• Wrote a basic test for the actual node use-case.

#105: drupal-1800998-105.patch queued for re-testing.

While fixing the remaining bugs I realized that: #1998182: Glossary view is broken

Removes the tag.

#111: drupal-1800998-111.patch queued for re-testing.

I have tested following cases and found no errors.

Node-views:

• Node-view with single term-id as argument as contextual filter works fine.
• Node-view with single term-name as argument as contextual filter works fine.
• Node-view with multiple term-ids as arguments as contextual filter works fine.
  Additional info: multiple arguments do not work completely with "," or "+" as conjunctors, but this seems to be another issue for me. Both working well using "all" as argument.
• Node-view with multiple term-names as arguments as contextual filter works fine.
  Additional info: Works only using "all" as argument.

User-based views:

• User-view with user-id as argument as contextual filter works fine.
• User-view with user-name as argument as contextual filter works fine.

Taxonomy-views:

• Term-based-view with term-id as argument as contextual filter works fine.
• Term-based-view with term-name as argument as contextual filter works fine.

pagers:

• Pager links in a node-based-view works fine.

In my opionion the patch works fine and can be committed.

Thank you very much!

Thanks for manually testing this.

The 20K of added tests in #105 is awesome! @dawehner++

Alex asked me to look at this. Certainly from a routing perspective I don't have anything to add as Crell has already given it the thumbs up and it looks pretty awesome to me. The only thing that jumped out at me was the Drupal::service() call in MenuLinkStorageController but I see there is already a separate issue to figure that general problem out over in #1909418: Allow Entity Controllers to have their dependencies injected.

So, +1 to RTBC :-)

Running the tests locally I get a fail...

View page controller test 9 passes, 0 fails, and 1 exception

Also we need to open follow up for...

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/PathPluginBase.phpundefined
@@ -34,6 +37,69 @@ protected function defineOptions() {
+    // @todo How do we apply argument validation?
+    $bits = explode('/', $this->getOption('path'));
+    // @todo Figure out validation/argument loading.
+    // Replace % with %views_arg for menu autoloading and add to the
+    // page arguments so the argument actually comes through.
+    $arg_counter = 0;

+++ b/core/modules/views/views.moduleundefined
@@ -690,6 +679,13 @@ function views_invalidate_cache() {
+  // Set the router to be rebuild.
+  // @todo Figure out why the cache rebuild is trigged but the route table
+  //   does not exist yet.
+  if (db_table_exists('router')) {
+    Drupal::service('router.builder')->rebuild();
+  }

For some odd reasons views_preprocess_html() now calls user_access, so the test needs database table now.

#2003864: Router rebuild is triggered before router table created.
#2003866: Invent argument validation in the router entries

#120: drupal-1800998-120.patch queued for re-testing.

Re-RTBC, the minor changes since #117 look great.

Committed 7002e6b and pushed to 8.x. Thanks!

adding tag

i am not sure whether we need a notification here? it is a straight conversion

yeah, other conversions are not getting individual change records.
this is part of #1971384: [META] Convert page callbacks to controllers
Let's just include this is the issues list. of https://drupal.org/node/1800686

restoring title