I have a D6 install that throws PHP notices whenever I disable JavaScript and use any batch API process. The problem is that _batch_progress_page_nojs() uses theme('maintenance_page') but does not call drupal_maintenance_theme() right before, which all other normal calls to that theme function do.

CommentFileSizeAuthor
#13 867722-batch-nojs-theme-maintenance-D6-2.patch10.23 KBDave Reid
#10 drupal-DRUPAL-6.batch-no-js.10.patch744 bytessun
#1 867722-batch-nojs-theme-maintenance-D6.patch679 bytesDave Reid
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented
Status: Active » Needs review
FileSize
867722-batch-nojs-theme-maintenance-D6.patch679 bytes

Simple one-line addition to prevent a slog of PHP notices.

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented

Same does not exist for D7 due to theme improvements by JohnAlbin and co.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

I'd like to see a reference and/or explanation for those theme improvements, as I'm unable to see where D7 gets this right(er).

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented

It was fixed by moving template_preprocess_maintenance_page into theme.inc where it was available to call. D7 issue for reference: #321828: maintenance_page not registered correctly in drupal_common_theme()

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs review » Reviewed & tested by the community

Thanks! That explanation makes this patch automatically RTBC.

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented
Issue tags: +pathauto, +xmlsitemap

Tagging my modules that are currently affected by this.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Reviewed & tested by the community » Fixed

Thanks, committed.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Priority: Normal » Critical
Status: Fixed » Needs work

This now enforces the maintenance theme to all batches in Drupal 6. That was not the case before. Any theme could have a maintenance page themed, so we should not necessarily set to use the maintenance theme here. Imagine a site having a front end batch process. With this patch and the front end theme not set to be also used as maintenance theme (pretty common I'd say), batches not broken and use Minelli. I'm considering this a regression.

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented

Ok then the other solution is to move theme_maintenance_page into includes/theme.inc. I'm not sure if there are any other hidden repercussions for doing that.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs work » Needs review
FileSize
drupal-DRUPAL-6.batch-no-js.10.patch744 bytes

nah, that means we only want to do what the wrapper function drupal_maintenance_theme() does, but not its innards.

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented

For some reason I thought I tried the above but it didn't work. I'll re-test.

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented
Status: Needs review » Needs work

Nope, it doesn't work. Still creates a bunch of notices.

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented
Status: Needs work » Needs review
FileSize
867722-batch-nojs-theme-maintenance-D6-2.patch10.23 KB

As I suspected, moving the preprocess function to theme.inc fixes the messages and will allow non-admin themes to be used for batches.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented

Hum, how including the file is different to moving the function to a file already included?

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

I suspect the theme registry doesn't recognize the preprocess function in the not included file when the theme registry is rebuilt, so regardless of whether the preprocess function is callable, the theme system does not know of it, and thus, doesn't try to invoke it.

Status: Needs review » Needs work

The last submitted patch, 867722-batch-nojs-theme-maintenance-D6-2.patch, failed testing.

dpearcefl’s picture

dpearcefl CreditAttribution: dpearcefl commented
Status: Needs work » Postponed (maintainer needs more info)

if this patch is still needed, please resubmit the patch with fixes.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Postponed (maintainer needs more info) » Needs work

@dpearceMN: Please stop doing this.

dpearcefl’s picture

dpearcefl CreditAttribution: dpearcefl commented

I'm just trying to gauge the interest in some issues. if a patch has been submitted and no action was taken to fix t he patch so it would be accepted, then the issue might be considered of little interest.

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented

Not true, so stop doing it.

Status: Needs work » Closed (outdated)

Automatically closed because Drupal 6 is no longer supported. If the issue verifiably applies to later versions, please reopen with details and update the version.