Updated: Comment #0

Problem/Motivation

As of #2089635: Convert non-test non-form page callbacks to routes and controllers, here is the following code for a simple route definition that is provided by dblog.module when search.module is enabled:

<?php

/**
 * @file
 * Contains \Drupal\dblog\Routing\RouteSubscriber.
 */

namespace Drupal\dblog\Routing;

use Drupal\Core\Extension\ModuleHandlerInterface;
use Drupal\Core\Routing\RouteBuildEvent;
use Drupal\Core\Routing\RoutingEvents;
use Symfony\Component\EventDispatcher\EventSubscriberInterface;
use Symfony\Component\Routing\Route;

/**
 * Provides dynamic routes for dblog.
 */
class RouteSubscriber implements EventSubscriberInterface {

  /**
   * The module handler.
   *
   * @var \Drupal\Core\Extension\ModuleHandlerInterface
   */
  protected $moduleHandler;

  /**
   * Creates a new RouteSubscriber.
   *
   * @param \Drupal\Core\Extension\ModuleHandlerInterface $module_handler
   *   The module handler.
   */
  public function __construct(ModuleHandlerInterface $module_handler) {
    $this->moduleHandler = $module_handler;
  }

  /**
   * {@inheritdoc}
   */
  public static function getSubscribedEvents() {
    $events[RoutingEvents::DYNAMIC] = 'routes';
    return $events;
  }

  /**
   * Generate dynamic routes for various dblog pages.
   *
   * @param \Drupal\Core\Routing\RouteBuildEvent $event
   *   The route building event.
   *
   * @return \Symfony\Component\Routing\RouteCollection
   *   The route collection that contains the new dynamic route.
   */
  public function routes(RouteBuildEvent $event) {
    $collection = $event->getRouteCollection();
    if ($this->moduleHandler->moduleExists('search')) {
      // The block entity listing page.
      $route = new Route(
        'admin/reports/search',
        array(
          '_content' => '\Drupal\dblog\Controller\DbLogController::search',
          '_title' => 'Top search phrases',
        ),
        array(
          '_permission' => 'access site reports',
        )
      );
      $collection->add('dblog.search', $route);
    }
  }

}

Here's how it could look (and live in the dblog.routing.yml file):

dblog.search:
  path: '/admin/reports/search'
  defaults:
    _title: 'Top search phrases'
    _content: '\Drupal\dblog\Controller\DbLogController::search'
  options:
    _module_exists: 'search'
  requirements:
    _permission: 'access site reports'

Proposed resolution

Add something to allow route definitions based on another module being enabled

Remaining tasks

Decide on an approach
Write tests

User interface changes

N/A

API changes

API addition

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

One question we should raise is: Are there really that many usecases for that that it is worth to add into core and not hardcode it for dblog?

Dave Reid’s picture

Yes. I often wrap menu callbacks in module_exists() in contrib modules: added info if Devel is enabled, a '404 results' in the Redirect module if the dblog module is enabled. I would find this very useful.

tim.plunkett’s picture

Well there is only one, but I can guarantee this will be used in contrib, like when a module adds new routes on behalf of a core module it might not depend on (like adding something to field_ui).

dawehner’s picture

Status: Active » Needs review
FileSize
1.88 KB

Here is just an idea: Provide a access checker which checks for the module name.

tim.plunkett’s picture

I think that will work fine for the ALL case, but it will present a problem when you want to use ANY as the access check mode.

Or, now as I reread the patch, is that what static::KILL is for?! Duh.

dawehner++

larowlan’s picture

Comment field patch adds it's own to check if node exists so this will simplify that

klausi’s picture

Status: Needs review » Needs work

An access checker is the wrong approach, that would be checked on every single page request. We need to modify the place where all the routes are collected from YAML and throw them out if the module does not exist. Or implement an event subscriber for RoutingEvents::ALTER that kicks out routes that have some _module_dependency key set and the module does not exist.

dawehner’s picture

An access checker is the wrong approach, that would be checked on every single page request.

Well, this is actually wrong. The applies method of the accessCheckers are called during building/dumping the routes to the DB,
in order to determine on which routes the access checker is used, which means that this additional access checker JUST runs on routes where it is supposed to exist.

On the other hand your suggestion feels a little bit cleaner, but would require more knowledge of the users (they are aware of access checkers at least).

damiankloip’s picture

Status: Needs work » Needs review

I was thinking exactly that, I just didn't see your comment, already working on a patch that does basically that.

We can then see the implementations and have a solid base to argue about :)

damiankloip’s picture

Assigned: Unassigned » damiankloip
Crell’s picture

Assigned: damiankloip » Unassigned
Status: Needs review » Needs work

Well, not THAT much more knowledge. They'd still use "options: _module_exists: views", or whatever, if we want to support this natively. The question is, do we. (This is, IMO, something that would be entirely safe to add post 8.0 so it's not a high priority. More nice-to-have.)

Klausi is right, though, that access checkers are the wrong approach. A failed access check will return a 403 error, when in fact what you want to get is a 404.

Crell’s picture

Assigned: Unassigned » damiankloip

Sorry, Damian.

damiankloip’s picture

Assigned: damiankloip » Unassigned
Status: Needs work » Needs review
FileSize
5.29 KB

Something like this maybe? Naming on all fronts up for discussion.

klausi’s picture

Status: Needs review » Needs work
  1. +++ b/core/lib/Drupal/Core/EventSubscriber/ModuleRouteSubscriber.php
    @@ -0,0 +1,60 @@
    +/**
    + * A route subscriber for altering routes based on module data.
    + */
    

    A bit obscure. Suggestion: "A route subscriber to remove routes that depend on a module being enabled"

  2. +++ b/core/lib/Drupal/Core/EventSubscriber/ModuleRouteSubscriber.php
    @@ -0,0 +1,60 @@
    +    foreach ($collection as $name => $route) {
    +      if ($route->hasRequirement('_module_dependency') && !$this->moduleHandler->moduleExists($route->getRequirement('_module_dependency'))) {
    +        $collection->remove($name);
    +      }
    +    }
    

    So this supports only single valued entries, correct? What if I want my route to depend on multiple modules being enabled?

And I think we should also start to convert the core use case of RouteSubscriber in dblog module to this new approach.

damiankloip’s picture

Yep, good points. That was just a start really... Will roll a new patch.

brianV’s picture

Just crossposting #2092529: [meta] Improve DX for defining custom routes as there is a fair amount of conceptual overlap between the two issues.

In general, we need a better way to specify routes dynamically. In practice, menu items in D7 contrib are dynamically specified based on everything from content types, to module status, to the phase of the moon in the current week. The current DX for these is bad; the narrow use-case this issue attempts to solve is merely one example.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
6.87 KB
9 KB

There is definitely work that can be done to improve the DX of dynamic routes, the issue in #16 does not make it much easier atm imo (sorry). Plugins should help with that though, and I think that is a totally legit thing to be improving on.

One reason I still think this is valid on it's own, is that the module_exists() check in hook_menu was by far the most common use of this type of thing. So I think it still makes sense to add something like this, so people can still just declare a route with a few lines of YAML (see the dblog.search conversion in this patch).

I think we have two things here: Dynamically adding routes, and actual dynamic routes. Examples like dblog.search do not really have anything dynamic about the route, just whether we want the route to be included or not, and things that rely on entity info etc.. will programmatically create each route, which are two distinct differences. If we only went with the plugin approach, I would still have to create a plugin and add an if($this->moduleHandler->moduleExists('meh')) etc.. to declare what is basically a static route like any other.

Status: Needs review » Needs work
Issue tags: -DX (Developer Experience), -MenuSystemRevamp, -WSCCI

The last submitted patch, 2091411-17.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
Issue tags: +DX (Developer Experience), +MenuSystemRevamp, +WSCCI

#17: 2091411-17.patch queued for re-testing.

Not sure I believe those.

dawehner’s picture

+++ b/core/tests/Drupal/Tests/Core/EventSubscriber/ModuleRouteSubscriberTest.php
@@ -0,0 +1,85 @@
+      ->method('moduleExists')
+      ->will($this->returnCallback(array($this, 'moduleExistsCallback')));

We could also just use returnValueMap, which seems to be easier to understand.

damiankloip’s picture

Good idea.

damiankloip’s picture

Assigned: Unassigned » damiankloip

Talked about this in IRC, going to make a few more changes.

Status: Needs review » Needs work

The last submitted patch, 2091411-21.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
5.56 KB
15.88 KB

OK, so after speaking to berdir yesterday, we decided:

- Move back to requirements instead of options
- Allow the same functionality that we have for roles with regards to AND/OR logic. Eg. _module_dependencies: 'search,dblog' / 'search+dblog'

damiankloip’s picture

Assigned: damiankloip » Unassigned
damiankloip’s picture

FileSize
10.02 KB

Whoops! Somehow added the interdiff with that last patch.

damiankloip’s picture

FileSize
491 bytes
10.01 KB

Sorry, forgot to change the dblog route back to us requirements.

dawehner’s picture

- Allow the same functionality that we have for roles with regards to AND/OR logic. Eg. _module_dependencies: 'search,dblog' / 'search+dblog'

Are there really usecase for an OR?

Berdir’s picture

Maybe, maybe not. But we thought that it is much easier to explain if all these options (roles, modules, permissions hopefully) use the same structure to define it.

dawehner’s picture

Status: Needs review » Needs work
  1. +++ b/core/lib/Drupal/Core/EventSubscriber/ModuleRouteSubscriber.php
    @@ -0,0 +1,99 @@
    +   * @param string $string
    +   * @param string $separator
    

    Needs a bit more docs.

  2. +++ b/core/tests/Drupal/Tests/Core/EventSubscriber/ModuleRouteSubscriberTest.php
    @@ -0,0 +1,84 @@
    +/**
    + * Tests the ModuleRouteSubscriber class.
    + *
    + * @see \Drupal\Core\EventSubscriber\ModuleRouteSubscriber
    + */
    +class ModuleRouteSubscriberTest extends UnitTestCase {
    

    It would be cool to have a @group Drupal on here.

  3. +++ b/core/tests/Drupal/Tests/Core/EventSubscriber/ModuleRouteSubscriberTest.php
    @@ -0,0 +1,84 @@
    +
    +    $collection = new RouteCollection();
    +    $route_1 = new Route('', array(), array('_module_dependencies' => 'enabled'));
    +    $collection->add('enabled', $route_1);
    +    $route_2 = new Route('', array(), array('_module_dependencies' => 'disabled'));
    +    $collection->add('disabled', $route_2);
    +    $route_3 = new Route('', array(), array('_module_dependencies' => 'disabled,enabled'));
    +    $collection->add('enabled_or', $route_3);
    +    $route_4 = new Route('', array(), array('_module_dependencies' => 'disabled,disabled'));
    +    $collection->add('disabled_or', $route_4);
    +    $route_5 = new Route('', array(), array('_module_dependencies' => 'disabled+disabled'));
    +    $collection->add('disabled_and', $route_5);
    +    $route_6 = new Route('', array(), array('_module_dependencies' => 'enabled+enabled'));
    +    $collection->add('enabled_and', $route_6);
    +    $route_7 = new Route('');
    +    $collection->add('no_module_dependencies', $route_7);
    +
    +    $event = new RouteBuildEvent($collection, 'test');
    +    $route_subscriber = new ModuleRouteSubscriber($module_handler);
    +    $route_subscriber->removeRoutes($event);
    +
    +    $this->assertInstanceOf('Symfony\Component\Routing\Route', $collection->get('enabled'));
    +    $this->assertInstanceOf('Symfony\Component\Routing\Route', $collection->get('enabled_or'));
    +    $this->assertInstanceOf('Symfony\Component\Routing\Route', $collection->get('enabled_and'));
    +
    +    $this->assertNull($collection->get('disabled'));
    +    $this->assertNull($collection->get('disabled_or'));
    +    $this->assertNull($collection->get('disabled_and'));
    +
    +    $this->assertInstanceOf('Symfony\Component\Routing\Route', $collection->get('no_module_dependencies'));
    +  }
    

    This seems to be a good usecase for a dataProvider

damiankloip’s picture

Status: Needs work » Needs review
FileSize
5.06 KB
10.04 KB

Thanks! Should have just used a data provider initially really..

klausi’s picture

Status: Needs review » Needs work
  1. +++ b/core/lib/Drupal/Core/EventSubscriber/ModuleRouteSubscriber.php
    @@ -0,0 +1,101 @@
    +            // If any moduleExists() call returns FALSE, remove the route move
    +            // onto the next.
    

    and move on to the next?

  2. +++ b/core/lib/Drupal/Core/EventSubscriber/ModuleRouteSubscriber.php
    @@ -0,0 +1,101 @@
    +        else {
    +          foreach ($this->explodeString($modules, ',') as $module) {
    +            $remove = TRUE;
    +            if ($this->moduleHandler->moduleExists($module)) {
    +              $remove = FALSE;
    +              break;
    +            }
    +          }
    +          // If no modules are found, remove the route.
    +          if ($remove) {
    +            $collection->remove($name);
    +          }
    +        }
    

    Ah, this is the OR case with "," separators? Please add a comment.

    And I think you can just call "continue 2;" here immediately as soon as you know that at least one module exists.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
1.4 KB
10.02 KB

Thanks, there we go.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Great patch!

jibran’s picture

Awesome work @damiankloip and @dawehner. I am unable to find any issue in code, doc or phpunit++.

+++ b/core/tests/Drupal/Tests/Core/EventSubscriber/ModuleRouteSubscriberTest.php
@@ -0,0 +1,97 @@
+      array('enabled_or',  array('_module_dependencies' => 'disabled,enabled'), FALSE),
+      array('disabled_or',  array('_module_dependencies' => 'disabled,disabled'), TRUE),
+      array('enabled_and',  array('_module_dependencies' => 'enabled+enabled'), FALSE),
+      array('disabled_and',  array('_module_dependencies' => 'disabled+disabled'), TRUE),

Why not add more use cases? Like three module dependencies?

array('enabled_or1',  array('_module_dependencies' => 'enabled,disabled'), FALSE),
array('enabled_or2',  array('_module_dependencies' => 'enabled,enabled'), FALSE),
array('disabled_and1',  array('_module_dependencies' => 'disabled+enabled'), TRUE),
array('disabled_and2',  array('_module_dependencies' => 'enabled+disabled'), TRUE),

And one more thing I think why not use & separator for AND and | for OR.
Other then this is pretty much RTBC for me.

dawehner’s picture

Status: Reviewed & tested by the community » Needs review

And one more thing I think why not use & separator for AND and | for OR.

We are using + and , in various places in core: Views argument splitting, multiple roles access checker etc., let's be consistent.

damiankloip’s picture

FileSize
1020 bytes
10.11 KB

For you jibran, let's add one more OR case that puts enabled before disabled :)

damiankloip’s picture

FileSize
1013 bytes
10.28 KB

Jibran wants the failing AND conditions too. Let's add those!

jibran’s picture

Status: Needs review » Reviewed & tested by the community

@damiankloip suggested in IRC that we'll not gain anything from adding more modules but @dawehner said let's add infinite number of modules for infinitive running test and I am in support of the idea RTBC for me so if this patch is yellow when committer tries to commit the only reason is because it has infinitive running test and we can't show infinitely long module list in the patch.

Crell’s picture

+++ b/core/lib/Drupal/Core/EventSubscriber/ModuleRouteSubscriber.php
@@ -0,0 +1,98 @@
+            // If any moduleExists() call returns FALSE, remove the route move
+            // on to the next.

Technically, "remove the route AND move on to the next." The "and" is missing. Trivial for Alex to fix while committing if he wants. :-)

Other than that, this patch gets a +1 from me.

damiankloip’s picture

FileSize
893 bytes
10.29 KB

Thanks, Crell :)

Let's change it quickly, just in case!

dawehner’s picture

Seriously nice work!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Now that #731724: Convert comment settings into a field to make them work with CMI and non-node entities is in we should remove Drupal\comment\Routing\RouteSubscriber here as well.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
1.22 KB
13.48 KB

Spoke to alexpott, we should also remove the RouteSubscriber from comment module.

Interdiff doesn't have file removal, it's in the patch though.

klausi’s picture

Status: Needs review » Reviewed & tested by the community

Cool, back to RTBC.

alexpott’s picture

Title: Provide an easier mechanism for a route definition wrapped by module_exists() » Change notice: Provide an easier mechanism for a route definition wrapped by module_exists()
Priority: Normal » Major
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Committed f366930 and pushed to 8.x. Thanks!

vijaycs85’s picture

Status: Active » Needs review

updated the documentation page here: https://drupal.org/node/1800686/revisions/view/2854193/2861303 please review.

dawehner’s picture

Are we sure that it is a good idea to put everything into a single change notice?

damiankloip’s picture

Yeah, this seems to be getting way too overloaded. This bit of functionality definitelyu seems worthy of its own change notice.

dawehner’s picture

44: 2091411-43.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 44: 2091411-43.patch, failed testing.

Crell’s picture

Issue summary: View changes
Status: Needs work » Fixed
Issue tags: -Needs change record

I think the current location is fine for the moment. Most of the content on that page will get turned into separate documentation anyway, a process that is already in, er, process.

Crell’s picture

Title: Change notice: Provide an easier mechanism for a route definition wrapped by module_exists() » Provide an easier mechanism for a route definition wrapped by module_exists()

Too many damned things to change...

Status: Fixed » Closed (fixed)

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