Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
Related Issues
Comment | File | Size | Author |
---|---|---|---|
#44 | 2091411-43.patch | 13.48 KB | damiankloip |
#44 | interdiff-2091411-43.txt | 1.22 KB | damiankloip |
#41 | 2091411-40.patch | 10.29 KB | damiankloip |
#41 | interdiff-2091411-40.txt | 893 bytes | damiankloip |
#38 | 2091411-38.patch | 10.28 KB | damiankloip |
Comments
Comment #1
dawehnerOne 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?
Comment #2
Dave ReidYes. 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.
Comment #3
tim.plunkettWell 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).
Comment #4
dawehnerHere is just an idea: Provide a access checker which checks for the module name.
Comment #5
tim.plunkettI 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 #6
larowlanComment field patch adds it's own to check if node exists so this will simplify that
Comment #7
klausiAn 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.
Comment #8
dawehnerWell, 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).
Comment #9
damiankloip CreditAttribution: damiankloip commentedI 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 :)
Comment #10
damiankloip CreditAttribution: damiankloip commentedComment #11
Crell CreditAttribution: Crell commentedWell, 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.
Comment #12
Crell CreditAttribution: Crell commentedSorry, Damian.
Comment #13
damiankloip CreditAttribution: damiankloip commentedSomething like this maybe? Naming on all fronts up for discussion.
Comment #14
klausiA bit obscure. Suggestion: "A route subscriber to remove routes that depend on a module being enabled"
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.
Comment #15
damiankloip CreditAttribution: damiankloip commentedYep, good points. That was just a start really... Will roll a new patch.
Comment #16
brianV CreditAttribution: brianV commentedJust 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.
Comment #17
damiankloip CreditAttribution: damiankloip commentedThere 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.
Comment #19
damiankloip CreditAttribution: damiankloip commented#17: 2091411-17.patch queued for re-testing.
Not sure I believe those.
Comment #20
dawehnerWe could also just use returnValueMap, which seems to be easier to understand.
Comment #21
damiankloip CreditAttribution: damiankloip commentedGood idea.
Comment #22
damiankloip CreditAttribution: damiankloip commentedTalked about this in IRC, going to make a few more changes.
Comment #24
damiankloip CreditAttribution: damiankloip commentedOK, 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'
Comment #25
damiankloip CreditAttribution: damiankloip commentedComment #26
damiankloip CreditAttribution: damiankloip commentedWhoops! Somehow added the interdiff with that last patch.
Comment #27
damiankloip CreditAttribution: damiankloip commentedSorry, forgot to change the dblog route back to us requirements.
Comment #28
dawehnerAre there really usecase for an OR?
Comment #29
BerdirMaybe, 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.
Comment #30
dawehnerNeeds a bit more docs.
It would be cool to have a @group Drupal on here.
This seems to be a good usecase for a dataProvider
Comment #31
damiankloip CreditAttribution: damiankloip commentedThanks! Should have just used a data provider initially really..
Comment #32
klausiand move on to the next?
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.
Comment #33
damiankloip CreditAttribution: damiankloip commentedThanks, there we go.
Comment #34
dawehnerGreat patch!
Comment #35
jibranAwesome work @damiankloip and @dawehner. I am unable to find any issue in code, doc or phpunit++.
Why not add more use cases? Like three module dependencies?
And one more thing I think why not use
&
separator for AND and|
for OR.Other then this is pretty much RTBC for me.
Comment #36
dawehnerWe are using + and , in various places in core: Views argument splitting, multiple roles access checker etc., let's be consistent.
Comment #37
damiankloip CreditAttribution: damiankloip commentedFor you jibran, let's add one more OR case that puts enabled before disabled :)
Comment #38
damiankloip CreditAttribution: damiankloip commentedJibran wants the failing AND conditions too. Let's add those!
Comment #39
jibran@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.
Comment #40
Crell CreditAttribution: Crell commentedTechnically, "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.
Comment #41
damiankloip CreditAttribution: damiankloip commentedThanks, Crell :)
Let's change it quickly, just in case!
Comment #42
dawehnerSeriously nice work!
Comment #43
alexpottNow 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.
Comment #44
damiankloip CreditAttribution: damiankloip commentedSpoke to alexpott, we should also remove the RouteSubscriber from comment module.
Interdiff doesn't have file removal, it's in the patch though.
Comment #45
klausiCool, back to RTBC.
Comment #46
alexpottCommitted f366930 and pushed to 8.x. Thanks!
Comment #47
vijaycs85updated the documentation page here: https://drupal.org/node/1800686/revisions/view/2854193/2861303 please review.
Comment #48
dawehnerAre we sure that it is a good idea to put everything into a single change notice?
Comment #49
damiankloip CreditAttribution: damiankloip commentedYeah, this seems to be getting way too overloaded. This bit of functionality definitelyu seems worthy of its own change notice.
Comment #50
dawehner44: 2091411-43.patch queued for re-testing.
Comment #52
Crell CreditAttribution: Crell commentedI 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.
Comment #53
Crell CreditAttribution: Crell commentedToo many damned things to change...