Blocks admin (admin/structure/block) requires menu_rebuild() to display settings for current theme

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

Patch same as #698406: block_menu() broken: depends on theme at time of menu cache rebuild

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.

For me this is RTBC

Crosslinking: #738958: Menu cache needs to be rebuilt when the default theme is changed

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

Now it's totally ready to commit!

Thanks bleen18, nice work.

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

+++ modules/system/system.test	15 Mar 2010 13:54:51 -0000
@@ -1329,6 +1329,26 @@ class SystemThemeFunctionalTest extends 
+    ¶

Trailing white-space.

Powered by Dreditor.

oops

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!

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...

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

Thanks!

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

Just checking if the patch still applies.

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

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

rerolled with fixed test

rerolled to HEAD

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

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

Closed #890894: Menu registry should be flushed when switching themes as duplicate.

-       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.

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

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.

Done

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

I can confirm that the 690544-rebuild-menu2.patch fix the issue

I've closed some duplicate of this bug:

http://drupal.org/node/1018916
http://drupal.org/node/884394
http://drupal.org/node/971186
http://drupal.org/node/1023114

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

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.

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.

+++ 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.

Don't fix indentation in this patch please :)

Done.

Re-rolled patch attached

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

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

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

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

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

Back to RTBC, well commented and test included

Thanks Robertom, it's could help !

tagging for backport.

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.

@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.

@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!

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.

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