Batch API pages should not show the toolbar

Here's a patch. Uses a straight arg(0) == 'batch' check in toolbar_page_build(), not sure if there are more elegant solutions.

Can I have a screenshot please? We want to avoid confusion by making stuff dissapear - its highly unlikely they will click away anyway.

Screenshot in http://drupal.org/node/539022#comment-1986000

I disagree about users clicking away being "unlikely". On the contrary - batch process pages don't have a cancel button (because 'cancel' is technically far from straightforward, different issue), and people might be tempted to navigate away to abort the process, which can do umdecidable harm.

That's exactly the reason batch pages don't show blocks: Same goes for the toolbar, it just shouldn't be here - batch page is not a page you can click away from.

As per #612974: Optimize toolbar to only build if it will be displayed, patch uses hook_page_alter() to set toolbar access to FALSE.

Note that the page callback system_batch_page() had some code to also *not* display blocks, but that seems to be broken with the D7 $page structure changes. Not for this issue.

Opened #614826: Batch page should not display blocks

+++ modules/system/system.module	26 Oct 2009 14:30:08 -0000
@@ -2995,6 +2995,16 @@
+function system_page_alter(&$page) {
+  // Do not display the toolbar on batch pages.
+  if (arg(0) == 'batch') {
+    $page['page_top']['toolbar']['#access'] = FALSE;
+  }
+}

I count the term "toolbar" two times here, but the function is system_page_alter().

Additionally, after moving to Toolbar module, I'd say that we want to prevent the entire building in the first place to make batches faster.

This review is powered by Dreditor.

ok, now I read and understood what happened in #612974: Optimize toolbar to only build if it will be displayed.

However, System module should not implement support code for other modules, especially, in case those other modules can happily be disabled. Instead, literally everything integrates with core system functionality. So this code has to be moved into toolbar.module.

I don't really care that much, but do we really prefer stuff like

$page['page_top']['toolbar'] = array(
  '#pre_render' => array('toolbar_pre_render'),
  '#access' => user_access('access toolbar') && (arg(0) != 'batch'),
  ...
);

in toolbar.module ? how many other &&s will we find we need to add later on ?
+ contrib modules will need to use the approach in patch #6 anyway. I think catch wrote in some thread something like 'we should write core modules as if they were in contrib', and that seems reasonable.

"System module should not implement support code for other modules"
Is it just because it's 'system' ? Batch pages happen to be provided by system.module, toolbar module doesn't need to know about them more than if they were provided by aggregator, right ?

Drupal core is System module: http://api.drupal.org/api/function/_drupal_maintenance_theme/7 (and Filter module currently, but I think I can file a patch to remove that)

All modules need to cater for the core system functionality, whether they want or not. But the core functionality cannot and should not cater for some arbitrary and ugly implemented, and totally optional feature that calls itself "toolbar".

So yeah, there are several options for Toolbar module:

1) Either don't add that stuff in the first place

2) Use #access to prevent rendering and invocation of #pre_render

3) Use toolbar_page_alter() to get rid of itself.

I'm with tha_sun: it's more logical for the toolbar to be aware of batch then the other way around.

This said, the batch API could #access => FALSE all the regions except content. Is there a reason not to do that?

This said, the batch API could #access => FALSE all the regions except content. Is there a reason not to do that?

If that's the right way to solve #614826: Batch page should not display blocks, then possibly. I must confess I did not closely follow the page rendering changes.