Regardless of what theme you choose to set blocks for, the block setting page saves settings for the current theme, since the elements include the current theme name as a hidden element, and the submit location is not inspected when storing data, only these hidden and backposted values.

A partial solution is presented here. I moved the custom theme check before these form items are generated, so that the proper theme name will be included in the form elements (and the selected theme will be previewed with its regions displayed).

Problems with my patch at least: 1. There is no reason to post to a custom URL anymore with the current aproach, I guess this can be removed altogether (easy). 2. If viewing the settings for the theme on the first subtab, due to the nature of the menu system, there is no way to get the theme name from arg(). How to get it? This one is not easy for me.

CommentFileSizeAuthor
#6 Drupal.fix-block-settings-theme-selection_0.patch2.57 KBnedjo
#3 Drupal.fix-block-settings-theme-selection.patch1.44 KBGábor Hojtsy
Drupal.fix-block-settings-partial.patch1.21 KBGábor Hojtsy
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Active » Needs work

This is a patch.

nedjo’s picture

nedjo
he/him/his
Primary language English
CreditAttribution: nedjo commented

Thanks for noting this issue. I haven't had a chance to test with a current CVS version to see the problem in action. Your solution seems to make sense.

As for problem #2, could we use something like 'callback arguments' => array($theme) in the menu declaration to feed this variable?

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Assigned: Unassigned » Gábor Hojtsy
Status: Needs work » Needs review
FileSize
Drupal.fix-block-settings-theme-selection.patch1.44 KB

Here is an updated patch, which (to me) seems to fix the problems I noted. My approach was to move the theme adjustment code before the form assembling code, as explained before, and also checking for the default theme, if there was no theme specified in the URL. Previously this bogusly meant that we deal with the current theme, but the current theme is not necceserily the default, and the site default theme is the menu default item, so sometimes we need to switch to the default theme too, when adjusting settings in a different theme.

Now this patch corrects the settings behaviour, but has one interesting side effect. If you click on admin/blocks, and you are not using the default theme, your view will immediately be switched to the default theme (as required by the regions visualisation feature), which might be quite unexpected for users. It might (or might not) be a good idea to make the current theme the default menu item, instead of the default theme.

Awaiting reviews.

nedjo’s picture

nedjo
he/him/his
Primary language English
CreditAttribution: nedjo commented

This bug is likely a result of the handling of theme regions being deleted in the Forms API changes, see bug http://drupal.org/node/34086. The patch given here may not be needed if the original problem is fixed.

Gábor Hojtsy’s picture

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

Nedjo, looking at the patch in #34086, I don't think these two problems are related, since what my patch does is to have proper theme names in the outputted forms, and this is not rectified with your patch AFAIS.

nedjo’s picture

nedjo
he/him/his
Primary language English
CreditAttribution: nedjo commented
Status: Needs review » Reviewed & tested by the community
FileSize
Drupal.fix-block-settings-theme-selection_0.patch2.57 KB

Right you are Goba. I've now had a chance to review this and can confirm (a) the issue exists as describes and (b) on my testing, the patch fully fixes the issue.

Here's a new patch version. I've just deleted a test in the menu generation for the status of the theme. This is no longer needed since list_themes() now returns only enabled (status = 1) themes.

Thanks for fixing this, Goba. This patch is ready to go and should be applied ASAP, so we can start to make up for lost time in theme designing.

colorado’s picture

colorado CreditAttribution: colorado commented
Status: Reviewed & tested by the community » Needs work

I applied the lastest patch above, and still have a problem: as long as bluemarine is enabled/checked (even when not set as the default) it overrides the selected default theme.

http://drupal.org/node/34086 seems to be a similar or same issue.

Bèr Kessels’s picture

Bèr Kessels CreditAttribution: Bèr Kessels commented
Status: Needs work » Closed (duplicate)

http://drupal.org/node/34086 fixed this

wpd’s picture

wpd CreditAttribution: wpd commented
Status: Closed (duplicate) » Active

This is still a problem with HEAD (as of today). You can only change blocks from Bluemarine. When you only have one theme enabled it works. When you add a new theme (or enable it) the currently enable blocks get copied to the new theme (I think).

The previous patch does not apply cleanly.

nedjo’s picture

nedjo
he/him/his
Primary language English
CreditAttribution: nedjo commented

In a clean install from today's HEAD, I was unable to reproduce the reported issues. The blocks worked as expected:

* When a new theme is enabled (for the first time), it gets the current default theme's blocks. They are assigned to the same region if that region is available in the newly-enabled theme, or if not then to that theme's default (first listed) region.
* In block admin we can set visibility and block assignment for each enabled theme.

wpd: did you start with a fresh database? If not, your errors may have reflected errors that occurred when you first enabled themes (with code that was, at that time, faulty).

Please try again with today's CVS HEAD and report back. If you still get errors, please describe in detail exactly what the errors are and also post the content of your blocks table here.

We need to confirm if this problem still exists and, if so, just what it is.

Thanks!

nedjo’s picture

nedjo
he/him/his
Primary language English
CreditAttribution: nedjo commented

If you have broken theme blocks/regions from a previous install but have upgraded your install to HEAD, you should be able to fix it by deleting the bad data. Something like:

* Make bluemarine the default theme, and disable all other themes.
* Execute SQL: "DELETE FROM blocks WHERE theme <> 'bluemarine';"

Then, when you enable new themes they should get the default theme's block settings.

chx’s picture

chx CreditAttribution: chx commented
Status: Active » Fixed

I am calling this fixed. Not closed, so that it can be reopened but if nedjo can't reproduce then it's fixed.

Permanently Undecided’s picture

Permanently Undecided CreditAttribution: Permanently Undecided commented
Status: Fixed » Active

I've been experiencing this bug with blocks created by views module.

I create a view with a block; the block appears as disabled in the blocks admin section. I enable it, but the database block entry is created only for the current theme, all other enabled themes are ignored, so when one changes themes the block created by views module disappears.

I didn't start out with a fresh installation, but I read nedjo's comment and so I disabled all other themes except the current one, saved, then reactivated them. It made no difference at all. I'd also like to point out that if instead I myself create a block, this doesn't happen and I get a database entry for every single enabled theme, so maybe it's a bug related to blocks created by modules.

I'm running drupal cvs, system_update_174.

Permanently Undecided’s picture

Permanently Undecided CreditAttribution: Permanently Undecided commented
Status: Active » Fixed

Forgive me, I'm a doofus and didn't apply the bluemarine fix. It seems to work now.

Anonymous’s picture

(not verified) CreditAttribution: commented
Status: Fixed » Closed (fixed)