Batch API should use the current theme to run the batches

It's been like that before Seven. Batch pages live at URL /batch. They are thus out of admin section, and use the regular frontend theme.

I'd tend to mark 'by design'.

#541148: Garland still used for all batch processes was marked duplicate.

Well, maybe we need a way to identify admin initiated batches and user initiated batches then? Ie. have an /admin/batch path and a /batch path which would only be different in what theme they use but otherwise map to the same function. When a batch is to be executed from an admin page, the admin batch path would be used?

I guess that's doable.
Then again, it would work only for admin theme vs main theme. There would still be a theme change if the batch is triggered from a page that uses a different front-end theme (rare case, I guess).

I'm wondering if the system_batch_page() callback can still initialize the page theme (i.e no theme call happened earlier on). Then batch_process() could just set $batch['theme'] = 'current_theme' and system_batch_page() would reuse that theme for the batch progress page ?

Another approach, now that jQueryUI is in core, would be to display batch progress in a jQ modal, which I always thought would be damn cool, and would somewhat sidestep the theme issue - but that's a lot more work...

Whatever the approach taken, I don't really have the time to look into it currently, any taker welcome :-/

An approach would be to save the current theme when batch_process is executed, and serialize it with the other process info. Then, retrieve the theme saved (the theme of the page that started the batch) and use it (setting the $custom_theme global
.
The attached patch implements this approach, and fixes the issue reported. The batch process page uses the same theme of the page that started the batch.

Wonderful. I'm not sure global's in the middle of functions are up to the current coding standards, but I love in this approach is that it is compatible with whatever theme switching the user had, so if there is section based theming, the batch will keep the same theme used for the section at hand, where she initiated a batch process.

Thanks for tackling this, dropcube !

Why 'theme_key' instead of just 'theme'. We don't suffix all our keys with '_key' ;-)

'theme_key' is already a global I think, can we use $GLOBALS['theme key'] instead?

Re-roling the patch against HEAD. Now using $GLOBALS['theme_key'] and 'theme' in the batch info.

I can't reproduce the bug reported by the bot. I can install Drupal, with the UI and with command line. Let's try with a fresh patch.

Patch is OK - can't test right now, but if dropcube confirms it works on, say, simpletest pages, then I think he can freely set RTBC.

Yes, I confirm it works on simpletest pages. See screenshot.

Hm, sorry, last detail: we shouldn't display the toolbar on those pages. We don't want to invite the users to navigate away while the batch is running, could leave a site in an undecidable/broken state.

@yched: I'd think that would be a different issue. Opened #563562: Batch API pages should not show the toolbar.

You're right. RTBC.

Nice improvement!

Committed to HEAD.

Either this fix doesn't work, or I'm doing something wrong in update status, but I'm not seeing this on a batch_process specified in the admin section (when you check manually for available updates using #597484-4: Use the Queue API to fetch available update data. Compare:

http://drupal.org/files/issues/597484-4.update_queue_api.fetch-manually-...
http://drupal.org/files/issues/597484-4.update_queue_api.fetch-manually-...

Any ideas? Maybe that patch is doing something wrong, but it sure appears that this fix isn't in place, even though I definitely have a clean install of HEAD I'm working from...

Thanks,
-Derek

Okay, a little debugging later and I confirmed that the code in the patch is both in my copy of core, and being hit. Inside system_batch_page(), if I dsm($GLOBALS['custom_theme']) it's coming out "seven". However, it's *definitely* themed as garland, not seven. ;) It's 3:30am here, and I'm fried, but if anyone has a chance to further debug, that'd be lovely...

Ahh, broken by #553944: Define hook_menu_get_item_alter() as a reliable hook that runs before the page is doomed. Setting $GLOBALS['custom_theme'] doesn't do any good anymore. ;)

yes, it's defined in the menu system now. Though I didn't know that $custom_theme doesn't work at all anymore. Which is a problem, because modules like http://drupal.org/project/switchtheme heavily rely on it. Either we re-open the other issue or we continue here, not sure.

Bumping priority.

So the first thing we definitely need is a test here. :P Had we had one, we would've seen that #553944: Define hook_menu_get_item_alter() as a reliable hook that runs before the page is doomed broke it before that got committed.

Though the lack of tests are somewhat at fault, it's mostly me :) I searched for all uses of $custom_theme throughout core while working on that patch at one point, but not recently enough to have caught the one introduced by this commit.

I'm somewhat consoled by the fact that the original patch that was committed here wouldn't have worked anyway :)... well, it worked fine with core, but a number of contrib modules would have conflicted with it and broken it, which is why we did #553944: Define hook_menu_get_item_alter() as a reliable hook that runs before the page is doomed in the first place.

Here's an initial start at a fix. This works, but does not come with tests (not sure off the top of my head how to write tests for the batch API, so deferring that for now). It involves more moving around of code than you might expect, because the theme is now set early in the page load, before the batch would otherwise be loaded from the database.

@sun: I think you may have a point. What we have in core now works fine for situations like this (where you are setting the theme for a particular page or set of pages) but does not work well for situations where you want to dynamically change the theme across the site. The overlay is another example of something that will run into problems here. I'll think about it a bit and reopen #553944: Define hook_menu_get_item_alter() as a reliable hook that runs before the page is doomed so we can make sure that capability gets added back ...

I'd be happy if some Batch API master could have a look at this patch.

+++ includes/batch.inc	12 Oct 2009 02:34:04 -0000
@@ -17,6 +17,24 @@
+ * @param $id
+ *  The ID of the batch to load.
+ * @return
+ *  An array representing the batch, if one was found.

Wrong indentation.

This review is powered by Dreditor.

I'm just a bit confused by this comment:

+function _system_batch_theme() {
+  // Retrieve the current state of the batch. In most cases the batch will not
+  // have been loaded yet.

I didn't follow the 'theme callback' thread, but I'm wondering in which cases the batch might be already loaded at this point ?

Other than that, patch looks fine.

Fixed the issue from #27 and also changed this from the previous patch:

if (!$batch) {
  require_once DRUPAL_ROOT . '/includes/batch.inc';
  if (isset($_REQUEST['id'])) {
    $batch = batch_load($_REQUEST['id']);
  }
}

to this:

if (!$batch && isset($_REQUEST['id'])) {
  require_once DRUPAL_ROOT . '/includes/batch.inc';
  $batch = batch_load($_REQUEST['id']);
}

because it's much cleaner - not sure what I was thinking the first time :)

@yched:

I didn't follow the 'theme callback' thread, but I'm wondering in which cases the batch might be already loaded at this point ?

Yeah, I'm not 100% sure either, which is why I left it a little wishy-washy in the code comment. The theme callback runs very early (before hook_init is called), so I guess the only way it could happen is if someone loaded the batch in hook_boot() or something like that, which I suppose would be pretty rare.

I'm putting this at "needs review" for the testbot, but we still do not have a test included in the patch, so it's not yet ready in that sense.

+++ includes/batch.inc	28 Oct 2009 05:02:57 -0000
@@ -17,6 +17,24 @@
+ * @param $id
+ *   The ID of the batch to load.

I had to think twice what the "id" of a batch is. Maybe we should clarify that this is a auto-assigned serial column id in the db? hrm, that suggestion sounds silly... So perhaps rather, that this id usually contained in $_GET['id']?

+++ includes/batch.inc	28 Oct 2009 05:02:57 -0000
@@ -17,6 +17,24 @@
+function batch_load($id) {
+  $batch = db_query("SELECT batch FROM {batch} WHERE bid = :bid AND token = :token", array(
+    ':bid' => $id,
+    ':token' => drupal_get_token($id))
+  )->fetchField();
+  if ($batch) {
+    return unserialize($batch);
+  }
+}

We should return FALSE at the end of the function here.

+++ modules/system/system.module	28 Oct 2009 05:02:58 -0000
@@ -1031,6 +1032,23 @@ function system_menu() {
+function _system_batch_theme() {
+  // Retrieve the current state of the batch. In most cases the batch will not
+  // have been loaded yet.

I'd prefer to drop the second sentence here. This code runs when the active menu handler is executed, so it's clear that anything before it will run before it, and if there's something that loaded it already, then it simply won't be loaded again, so the "wishy-washy" comment (as you put it) has no point in itself ;)

I'm on crack. Are you, too?

This sounds more like a feature request. Important, nonetheless.

Actually, since a first patch already got in, got broken by a later commit, then turned out to be flawed anyway, this is now technically a bug ;-)

This is an annoying regression, but no totally critical bug - hence, changing tags, priority, and also assignment, because it seems like dropcube is no longer working on it.

OK, this fixes the three points made in #30, and also adds a test.

As explained in the code comment, there doesn't seem to be a way to test this 100% directly, so I used a slightly indirect method - however, it works fine (in that if you run the test without applying the rest of the patch, the test correctly fails and therefore identifies the bug).

Code is fine by me, sun's concerns were adressed.
drupalPost/drupalGet indeed loop through the batch redirects, and thus do not let tests 'see' the batch page. The workaround is smart and tests pass.
RTBC. Thanks David.

While people are looking at the batch progress page:
#563562: Batch API pages should not show the toolbar should be ready and needs someone to push the RTBC button.
#614826: Batch page should not display blocks could use some feedback on page rendering strategies.

Awesome, thanks a lot! With tests, even. :D

Committed to HEAD.

We have the same problem with admin/reports/status/run-cron, which inherits the theme settings from admin_theme because of admin in path. This is a very simple fix, see attached patch, but for those just reading and not applying, the change is:

  $items['admin/reports/status/run-cron'] = array(
    ...
    'theme callback' => 'variable_get',
    'theme arguments' => array('theme_default'),
  );

Unless I'm missing something, Drupal does not have a variable called 'theme'... :) Also, I don't understand - why does it matter what theme is used during a cron run?

Good catch, should be 'theme_default'.

It matters because what people do in hook_cron may very likely rely on the base theme. You might use hook_cron to prebuild cached pages. You might use hook_cron to send out emails that rely on your theme. You can use hook_cron to do anything, and certainly, you'd expect your theme overrides in the base to be called.

Also, you'll get different themes, running run-cron in admin and cron.php from cron, and is bound to cause problems. The point of the run-cron link is to run cron manually, not run it differently.

Yeah for example search indexing does a full comment render which will invoke templates, so this needs to be the site theme. Looks good to me.

Can this get a code comment explaining the reasoning here? I think it's probably non-obvious for someone looking at the code.

Also, this same issue exists in D6 as far as I know - so would be interesting to see if there are any real-world cases where this caused someone a problem :) But the examples in #40 do seem very realistic, especially the idea of sending out templated emails or that kind of thing.

Really, a comment for this? We don't have comments anywhere else we set the theme. IMHO, the extra comment isn't worth it.

The real world example where this caused me pain is the templated emails.

Here's the patch with the comment ... please feel free to edit the comment as you like.

Agree on adding the comment, less likely someone tidies this up later and breaks it again. Can't think of a better comment that wouldn't be a paragraph long, and this conveys the information we need, so re-rtbcing.

Yeah, I think the reason a comment is worthwhile is that this menu callback doesn't result in anything actually being displayed on the screen, so people would definitely wonder why we need to bother fiddling with the theme for it!

The comment looks good to me too.

I'm not sure about this fix.

What happens when someone calls drupal_cron_run() from elsewhere? Shouldn't it be the task of drupal_cron_run() to set the proper theme? I'd like us to think about this some more.

Or, if a cron function depends on a specific theme, it sounds like it should be that cron function's responsibility to make sure it is set to be the active theme.

I also don't understand why search module would require a specific theme (per catch's example)

I think by the time you get to drupal_cron_run() or later it is potentially too late - the theme may have already been initialized by someone else... and once it's initialized, Drupal doesn't exactly make it easy to undo that (http://api.drupal.org/api/function/drupal_theme_initialize/7), so without expanding this issue a lot I'm not sure if there's anything else that can be done.

http://api.drupal.org/api/function/comment_node_update_index/7

calls drupal_render()

drupal_render() calls theme()

If you are hiding labels or field values or whatever in your theme, you wouldn't want those showing up in your search index either. Same goes for indexing nodes - IMO node templates should be allowed to run.

Here is a patch to simpletest.test to verify that this issue is fixed.

Oops, forgot to remove the first line. This is my first patch, all apologies. Resubmitting the patch.

Subscribing, I'm facing similar problems: #981654: Use several themes during the same page request

Just a reroll to head and to git. Moving to d8 as it needs to be fixed there first.

#55: run-cron-539022-55.patch queued for re-testing.

#29: batch-theme-fix-539022-29.patch queued for re-testing.

#55: run-cron-539022-55.patch queued for re-testing.

// Use the same theme as the page that started the batch.
IMHO we shouldn't only use the same theme as the page that started the batch. We should rather really stay on the path of the page that started the batch, just the way install.php, update.php - or more generally Better Batch module - work.
We could and probably should also leverage modals using the new Dialog API.

Batch progress in popups would totally be nifty.
I opened #1993518: Display batch progress in modals for this. Needs volunteers :-)

Thanks, Yves, yes that'd be awesome!
Batches in modals don't completely supersede this issue, but that's where the music is playing.
We shouldn't forget to return here afterwards, though.

Wrong route name used in BatchNegotiator::applies method.
Patch attached.

The last patch, in #71, identifies an incorrect route in BatchNegotiator.php. I searched for duplicates of that didn't find any. I'll make an issue for this. I've made an issue for this, #3221633: BatchNegotiator tests for an invalid route. Added credit for Zemelia over there.

This was committed in Nov 2009, #36. It was then reopened, I think, to discuss a similar problem with the theme when cron is run manually. There is no commit related to that and I am not sure how to test it to determine if it still a problem.

Is this still a problem? Should this be closed, fixed, and a new issue opened to discuss the theme with cron or just closed?

Discussed with @catch in #bugsmash and we agreed that manually running cron shouldn't be an issue any longer since that doesn't run in a batch anymore.

@quietone has opened a follow up for the one remaining issue that was raised here.

So moving back to 'fixed' since this was committed back in 2009.

There is an issue with "applies" method of BatchNegotiator class. This doesn't take the "system.batch_page.json" route into account. So for example while the default theme is used on the "system.batch_page.html" route the admin theme is used on "system.batch_page.json" route. Question is why "system.batch_page.json" route is not taken into account in the "applies" method? In my opinion this should be consistent through the whole batch process regardless of this is the batch page("system.batch_page.html") or the batch ajax calls("system.batch_page.json").

Also the following is how the theme is set for the batch at the beginning.

function batch_process($redirect = NULL, Url $url = NULL, $redirect_callback = NULL) {
  $batch =& batch_get();

  $activeThemeName = \Drupal::service('theme.manager')->getActiveTheme();
  $request = \Drupal::request();

  if (isset($batch)) {
    // Add process information
    $process_info = [
      'current_set' => 0,
      'progressive' => TRUE,
      'url' => $url ?? Url::fromRoute('system.batch_page.html'),
      'source_url' => Url::fromRouteMatch(\Drupal::routeMatch())->mergeOptions(['query' => \Drupal::request()->query->all()]),
      'batch_redirect' => $redirect,
      'theme' => \Drupal::theme()->getActiveTheme()->getName(),
      'redirect_callback' => $redirect_callback,
    ];

=>

'theme' => \Drupal::theme()->getActiveTheme()->getName(),

So this should apply to the ajax calls too.

I created a patch for this issue.

Pls repoen the issue.