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.right now, this is the essence of the page manager menu delegation logic:
$tasks = page_manager_get_tasks();
// Provide menu items for each task.
foreach ($tasks as $task_id => $task) {
$handlers = page_manager_get_task_handler_plugins($task);
// Allow the task to add its own menu items.
if ($function = ctools_plugin_get_function($task, 'hook menu')) {
$function($items, $task);
}
// And for those that provide subtasks, provide menu items for them, as well.
foreach (page_manager_get_task_subtasks($task) as $subtask_id => $subtask) {
// Allow the task to add its own menu items.
if ($function = ctools_plugin_get_function($task, 'hook menu')) {
$function($items, $subtask);
}
}
}
first, a simple problem - why are we running page_manager_get_task_handler_plugins($task); at all? we don't pass those $handlers anywhere, and that function doesn't do anything by reference. seems like cruft, unless the point is loading all the handlers in order to reveal their own hook_menu implementations for delegated inclusion. i don't see where that would happen, though.
bigger problem is this, though:
// And for those that provide subtasks, provide menu items for them, as well.
foreach (page_manager_get_task_subtasks($task) as $subtask_id => $subtask) {
// Allow the task to add its own menu items.
if ($function = ctools_plugin_get_function($task, 'hook menu')) {
we're iterating over the subtasks, but still searching for the plugin function on the parent task. which means the parent's hook menu gets called once for each of the subtasks it has. yikes. my initial thought was to just change this, but it seems like the page plugin is indeed adding more on each passthrough.
i could relatively easily work around this for my purposes simply by using the hook menu alter callback instead, as it has neither of the problems that hook menu suffers from:
$tasks = page_manager_get_tasks();
foreach ($tasks as $task) {
if ($function = ctools_plugin_get_function($task, 'hook menu alter')) {
$function($items, $task);
}
// let the subtasks alter the menu items too.
foreach (page_manager_get_task_subtasks($task) as $subtask_id => $subtask) {
if ($function = ctools_plugin_get_function($subtask, 'hook menu alter')) {
$function($items, $subtask);
}
}
}
however, this seems like an api consistency/sanity issue, so i thought i'd raise it in a permanent forum so we can more easily figure out the course of action we want to take.
Comments
Comment #1
sdboyer CreditAttribution: sdboyer commentedactually, i was wrong - the page hook_menu implementation doesn't seem to add anything beyond the first one. so it seems like we're just reprocessing everything on menu rebuild for no particular reason :)
so barring any concerns, i'm gonna fix those two lines to be the exact same for
hook menuas forhook menu alter.Comment #2
merlinofchaos CreditAttribution: merlinofchaos commentedWell, switching to alter changes the signature for any existing subtasks outside of CTools. I realize there is a low probability of them existing. But that's one thing to consider.
Changing to an alter changes the order the items appear. That could potentially be an issue for other things also doing an alter for page manager items.
I do agree that fetching the handlers seems to be cruft.
You mention 'workaround' which means you have an actual problem? Because the only problem you've identified is inefficient code and maybe poor DX, but these arent things you need to work around.
Comment #3
sdboyer CreditAttribution: sdboyer commentedOK, i've pushed a commit removing the crufty call, for starters. as for the other bit, transcript of our irc conversation:
so, gonna mark this as postponed until we decide how we want to handle this mini-breakage for api consumers.