Expected behavior

The admin/structure/block config screen should, by default, present the block set-up for the currently running theme.

Current behavior as of alpha 1

The admin/structure/block requires a cache clear if the theme has been changed from the one set up in the initial install script.

From a usability perspective I think this is pretty big. New people to Drupal testing out D7 and changing themes will likely bang their heads causing many bruises :)

Shai
I've set up a Drupal 7 sandbox at http://d7sandbox.net

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dev.cmswebsiteservices’s picture

Assigned: Unassigned » dev.cmswebsiteservices
Status: Active » Needs review
FileSize
425 bytes

I have updated the system.admin.inc so now that rebuild the menu system on selection of default theme.
Working well on our test linux and windows server.
Thanks

Status: Needs review » Needs work

The last submitted patch, 690544-1.patch, failed testing.

dev.cmswebsiteservices’s picture

Status: Needs work » Needs review
FileSize
650 bytes

Status: Needs review » Needs work

The last submitted patch, system-690544-3.patch, failed testing.

dev.cmswebsiteservices’s picture

FileSize
635 bytes
dev.cmswebsiteservices’s picture

Status: Needs work » Needs review
andypost’s picture

marcvangend’s picture

Thanks andypost for noticing the duplicate.

Allow me to re-post my patch from the other issue here. IMHO it is slightly better because it also adds a comment explaining why the menu is being rebuilt. It also corrects a missing space a couple of lines earlier.

aspilicious’s picture

Version: 7.0-alpha1 » 7.x-dev
Assigned: dev.cmswebsiteservices » Unassigned

For me this is RTBC

aspilicious’s picture

bleen’s picture

FileSize
3.09 KB

this is the patch from #738958: Menu cache needs to be rebuilt when the default theme is changed ... it is the identical solution as #8 but with some tests

andypost’s picture

Status: Needs review » Reviewed & tested by the community

Now it's totally ready to commit!

marcvangend’s picture

Thanks bleen18, nice work.

bleen’s picture

Component: block.module » system.module

no prob! ... (also changing component to problem instead of symptom)

sun’s picture

Status: Reviewed & tested by the community » Needs review
+++ modules/system/system.test	15 Mar 2010 13:54:51 -0000
@@ -1329,6 +1329,26 @@ class SystemThemeFunctionalTest extends 
+    ¶

Trailing white-space.

Powered by Dreditor.

bleen’s picture

FileSize
3.08 KB

oops

sun’s picture

Sorry, I needed another coffee...

+++ modules/system/system.test	15 Mar 2010 13:54:51 -0000
@@ -1329,6 +1329,26 @@ class SystemThemeFunctionalTest extends 
+   * Test switching the deafult theme.

Typo in "deafult"

+++ modules/system/system.test	15 Mar 2010 13:54:51 -0000
@@ -1329,6 +1329,26 @@ class SystemThemeFunctionalTest extends 
+    $this->clickLink(t('Set default'), 1);
...
+    $this->clickLink(t('Set default'), 0);

Hm. This clicking of links by index is a bit fragile. I think we can replace this with direct drupalGet()s of the target URLs, because if they don't work, the following assertions will fail.

+++ modules/system/system.test	15 Mar 2010 13:54:51 -0000
@@ -1329,6 +1329,26 @@ class SystemThemeFunctionalTest extends 
+    $this->assertText('Stark (' . t('active tab') . ')', t('Active menu item on blocks admin page is the default theme.'));
...
+    $this->assertText('Garland (' . t('active tab') . ')', t('Active menu item on blocks admin page has changed.'));

The messages should rather refer to the "Default local task on..."

144 critical left. Go review some!

Status: Needs review » Needs work

The last submitted patch, 690544.patch, failed testing.

bleen’s picture

Hm. This clicking of links by index is a bit fragile. I think we can replace this with direct drupalGet()s of the target URLs, because if they don't work, the following assertions will fail.

The problem is that these particular links have form tokens attached to them. I am not sure how to handle that in test. Any advice?

The other notes are simple enough...

bleen’s picture

Status: Needs work » Needs review
FileSize
3.09 KB

this patch fixes the typo sun pointed out as well as the messages about "default local task." The tests that click links by index remain (see #19)

I also cannot fathom why #17 failed - I think testbot might be drunk agian

sun’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!

marcvangend’s picture

#20: 690544.patch queued for re-testing.

Just checking if the patch still applies.

bleen’s picture

#20: 690544.patch queued for re-testing.

aspilicious’s picture

#20: 690544.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 690544.patch, failed testing.

bleen’s picture

Status: Needs work » Needs review
FileSize
3.06 KB

rerolled with fixed test

dbeheydt’s picture

FileSize
2.73 KB

rerolled to HEAD

Status: Needs review » Needs work

The last submitted patch, 690544_3.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review

#26: 690544.patch queued for re-testing.

rschwab’s picture

FileSize
3.08 KB

Oops I think you guys meant Bartik! Trying that in the test.

rschwab’s picture

EvanDonovan’s picture

Title: admin/structure/block Requires Cache Clear in Order to Display Settings for Current Theme » Blocks admin (admin/structure/block) requires menu_rebuild() to display settings for current theme
Priority: Normal » Major
Status: Needs review » Needs work
-       theme_enable(array($theme));
+        theme_enable(array($theme));

Whitespace error.

// Rebuild the menu cache so that theme related menu items are refreshed.

Should be "theme-related". Also, might want to consider the comments from #950770-10: Contributed themes block configuration tab misnamed/does not show when multiple themes enabled; I flagged that as duplicated.

Other than that, this looks like it should be good (code-wise).

I think this is major, and should be fixed before Jan. 5, since it has a lot of potential to confuse site admins.

andypost’s picture

Status: Needs work » Needs review
FileSize
3.35 KB

Re-roll with proposed comment #950770-10: Contributed themes block configuration tab misnamed/does not show when multiple themes enabled

PS: theme_enable() hunk is whitespace fix

EvanDonovan’s picture

Status: Needs review » Needs work

Oh, I see. Sorry for mistakenly thinking that the whitespace was wrong in that part.

I realized after seeing it again that the comment is grammatically incorrect (punctuation).

+
+      // Rebuild the menu. This duplicates the menu_rebuild() in theme_enable()
+      // however modules must know the current default theme in order to use
+      // this information in hook_menu() or hook_menu_alter() implementations.
+      // And doing the variable_set() before the theme_enable() could result
+      // in a race condition where the theme is default but not enabled.
+      menu_rebuild();
+

Should be

+
+      // Rebuild the menu. This duplicates the menu_rebuild() in theme_enable().
+      // However, modules must know the current default theme in order to use
+      // this information in hook_menu() or hook_menu_alter() implementations,
+      // and doing the variable_set() before the theme_enable() could result
+      // in a race condition where the theme is default but not enabled.
+      menu_rebuild();
+

I promise I'll test the re-roll, although should be RTBC based on @sun's earlier review.

andypost’s picture

Status: Needs work » Needs review
FileSize
3.36 KB

Done

Status: Needs review » Needs work

The last submitted patch, 690544-rebuild-menu2.patch, failed testing.

rschwab’s picture

Status: Needs work » Needs review

#35: 690544-rebuild-menu2.patch queued for re-testing.

robertom’s picture

marcvangend’s picture

#35: 690544-rebuild-menu2.patch queued for re-testing.

tumblingmug’s picture

This bug is well-known at least for one year. Then D7-stable has been released containing it.
The bug itself is obvious and annoying; my favorite screenshot here: #884394: Regression: current active theme not selected when going to admin/structure/block

A comment would be helpful why an available patch is not committed to dev until now?! Is this bug going to survive the Drupal 7.1 release?

(Although I think specifically this patch should maybe not be commited, because its probably not up to the system module to solve problems of the block module, which itself introduces dynamic local tasks and does not worry about menu caching.)

I suggest to commit the patch and mark the inserted line as a removable one for later, after the block module has fixed this.

marcvangend’s picture

tumblingmug: one obvious reason it hasn't been committed, is that so far no-one has set this issue to "reviewed & tested by the community". If you have the time to test the patch and see if everything works as expected, please do.

I see your point regarding system module solving problems of the block module, but the inline comment clearly explains why it's not an option to call menu_rebuild() from block_themes_enabled(). That said, this patch is a significant improvement over the current situation, so if it works as advertised, I think it should be committed ASAP.

joachim’s picture

Status: Needs review » Needs work
+++ modules/system/system.admin.inc	26 Dec 2010 23:23:46 -0000
@@ -346,10 +346,18 @@ function system_theme_default() {
-       theme_enable(array($theme));
+        theme_enable(array($theme));

Don't fix indentation in this patch please :)

Powered by Dreditor.

robertom’s picture

FileSize
2.84 KB

Don't fix indentation in this patch please :)

Done.

Re-rolled patch attached

robertom’s picture

Status: Needs work » Needs review
robertom’s picture

Status: Needs review » Reviewed & tested by the community

tumblingmug: one obvious reason it hasn't been committed, is that so far no-one has set this issue to "reviewed & tested by the community". If you have the time to test the patch and see if everything works as expected, please do.

I've tested this patch at #38 and now. It seems to works well.

I set this patch as "reviewed & tested by the community" because the "core" of patch is reviewed also by sun at #21, and the last change is only a better documentation and test

bleen’s picture

Status: Reviewed & tested by the community » Needs review

robertm: you cannot set an issue to RTBC if you submitted the patch in question ... unfortunately I cannot either because I wrote a good chunk of this patch

tumblingmug’s picture

Status: Needs review » Reviewed & tested by the community

At the end I agree with marcvangend (#41) and have tested the patch sucessfully.

rfay’s picture

Version: 7.x-dev » 8.x-dev
Status: Reviewed & tested by the community » Needs review

Sorry. D8 first. Let's get these fixed on D8 and into D7.

bleen’s picture

#43: 690544-rebuild-menu3.patch queued for re-testing.

andypost’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC, well commented and test included

Dood59’s picture

Thanks Robertom, it's could help !

catch’s picture

Issue tags: +Needs backport to D7

tagging for backport.

tumblingmug’s picture

I love #48: "yeah, we could fix it, but we don't, because let's think of D8!" - So people stay using D6 because of buggy D7 to which we deny applying bug fixes because of D8.
Oh, I see the pure developer perspective and you must follow the guidelines! But what a funny kind of inflexible workflow from a user perspective.

catch’s picture

@tumblingmug: please read all comments on #1050616: Figure out backport workflow from Drupal 8 to Drupal 7 before making uninformed comments full of FUD. I don't see your username on that issue.

tumblingmug’s picture

@catch Thanks for the useful link. I havn't noticed this discussion yet. And I feel better, that even developers like fago do not love this workflow. He and others are describing exactly my experience with this issue. So definetely useful to read there to understand here. Thanks again!

catch’s picture

I don't think anyone loves the workflow. In general it has not been working well since 7.0 release due to a very slow rate of commits, as well as general confusion over the workflow, but those two issues are not inherent to the current workflow, so I am more interested in trying to get those sorted out.

Dries’s picture

Great. Committed this patch to 7.x and 8.x. Thanks everyone for your participation.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)
Issue tags: -Needs backport to D7

Automatically closed -- issue fixed for 2 weeks with no activity.