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

Files: 
CommentFileSizeAuthor
#44 2091411-43.patch13.48 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 2091411-43.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#44 interdiff-2091411-43.txt1.22 KBdamiankloip
#41 2091411-40.patch10.29 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 58,646 pass(es).
[ View ]
#41 interdiff-2091411-40.txt893 bytesdamiankloip
#38 2091411-38.patch10.28 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 58,696 pass(es).
[ View ]
#38 interdiff-2091411-38.txt1013 bytesdamiankloip
#37 2091411-37.patch10.11 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 58,350 pass(es).
[ View ]
#37 interdiff-2091411-37.txt1020 bytesdamiankloip
#33 2091411-33.patch10.02 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 58,683 pass(es).
[ View ]
#33 interdiff-2091411-33.txt1.4 KBdamiankloip
#31 2091411-31.patch10.04 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] 58,748 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#31 interdiff-2091411-31.txt5.06 KBdamiankloip
#27 2091411-27.patch10.01 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 58,584 pass(es).
[ View ]
#27 interdiff-2091411-27.txt491 bytesdamiankloip
#26 2091411-25.patch10.02 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#24 2091411-24.patch15.88 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#24 interdiff-2091411-24.txt5.56 KBdamiankloip
#21 2091411-21.patch8.71 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] 58,670 pass(es), 6 fail(s), and 0 exception(s).
[ View ]
#21 interdiff-2091411-21.txt1.5 KBdamiankloip
#17 2091411-17.patch9 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 59,223 pass(es).
[ View ]
#17 interdiff-2091411-17.txt6.87 KBdamiankloip
#13 2091411-11.patch5.29 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 59,514 pass(es).
[ View ]
#4 module_exists-2091411-4.patch1.88 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 59,195 pass(es).
[ View ]

Comments

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?

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.

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).

Status:Active» Needs review
StatusFileSize
new1.88 KB
PASSED: [[SimpleTest]]: [MySQL] 59,195 pass(es).
[ View ]

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

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++

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

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.

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).

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 :)

Assigned:Unassigned» damiankloip

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.

Assigned:Unassigned» damiankloip

Sorry, Damian.

Assigned:damiankloip» Unassigned
Status:Needs work» Needs review
StatusFileSize
new5.29 KB
PASSED: [[SimpleTest]]: [MySQL] 59,514 pass(es).
[ View ]

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

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.

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

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.

Status:Needs work» Needs review
StatusFileSize
new6.87 KB
new9 KB
PASSED: [[SimpleTest]]: [MySQL] 59,223 pass(es).
[ View ]

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.

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.

+++ 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.

StatusFileSize
new1.5 KB
new8.71 KB
FAILED: [[SimpleTest]]: [MySQL] 58,670 pass(es), 6 fail(s), and 0 exception(s).
[ View ]

Good idea.

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.

Status:Needs work» Needs review
StatusFileSize
new5.56 KB
new15.88 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

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'

Assigned:damiankloip» Unassigned

StatusFileSize
new10.02 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Whoops! Somehow added the interdiff with that last patch.

StatusFileSize
new491 bytes
new10.01 KB
PASSED: [[SimpleTest]]: [MySQL] 58,584 pass(es).
[ View ]

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

- 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?

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.

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

Status:Needs work» Needs review
StatusFileSize
new5.06 KB
new10.04 KB
FAILED: [[SimpleTest]]: [MySQL] 58,748 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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

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.

Status:Needs work» Needs review
StatusFileSize
new1.4 KB
new10.02 KB
PASSED: [[SimpleTest]]: [MySQL] 58,683 pass(es).
[ View ]

Thanks, there we go.

Status:Needs review» Reviewed & tested by the community

Great patch!

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.

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.

StatusFileSize
new1020 bytes
new10.11 KB
PASSED: [[SimpleTest]]: [MySQL] 58,350 pass(es).
[ View ]

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

StatusFileSize
new1013 bytes
new10.28 KB
PASSED: [[SimpleTest]]: [MySQL] 58,696 pass(es).
[ View ]

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

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.

+++ 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.

StatusFileSize
new893 bytes
new10.29 KB
PASSED: [[SimpleTest]]: [MySQL] 58,646 pass(es).
[ View ]

Thanks, Crell :)

Let's change it quickly, just in case!

Seriously nice work!

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.

Status:Needs work» Needs review
StatusFileSize
new1.22 KB
new13.48 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 2091411-43.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

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

Status:Needs review» Reviewed & tested by the community

Cool, back to RTBC.

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!

Status:Active» Needs review

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

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

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

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

Status:Needs review» Needs work

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

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.

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.