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
Comment | File | Size | Author |
---|---|---|---|
#43 | 690544-rebuild-menu3.patch | 2.84 KB | robertom |
#35 | 690544-rebuild-menu2.patch | 3.36 KB | andypost |
#33 | 690544-rebuild-menu.patch | 3.35 KB | andypost |
#30 | 690544_4.patch | 3.08 KB | rschwab |
#27 | 690544_3.patch | 2.73 KB | dbeheydt |
Comments
Comment #1
dev.cmswebsiteservices CreditAttribution: dev.cmswebsiteservices commentedI 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
Comment #3
dev.cmswebsiteservices CreditAttribution: dev.cmswebsiteservices commentedComment #5
dev.cmswebsiteservices CreditAttribution: dev.cmswebsiteservices commentedComment #6
dev.cmswebsiteservices CreditAttribution: dev.cmswebsiteservices commentedComment #7
andypostPatch same as #698406: block_menu() broken: depends on theme at time of menu cache rebuild
Comment #8
marcvangendThanks 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.
Comment #9
aspilicious CreditAttribution: aspilicious commentedFor me this is RTBC
Comment #10
aspilicious CreditAttribution: aspilicious commentedCrosslinking: #738958: Menu cache needs to be rebuilt when the default theme is changed
Comment #11
bleen CreditAttribution: bleen commentedthis 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
Comment #12
andypostNow it's totally ready to commit!
Comment #13
marcvangendThanks bleen18, nice work.
Comment #14
bleen CreditAttribution: bleen commentedno prob! ... (also changing component to problem instead of symptom)
Comment #15
sunTrailing white-space.
Powered by Dreditor.
Comment #16
bleen CreditAttribution: bleen commentedoops
Comment #17
sunSorry, I needed another coffee...
Typo in "deafult"
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 messages should rather refer to the "Default local task on..."
144 critical left. Go review some!
Comment #19
bleen CreditAttribution: bleen commentedThe 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...
Comment #20
bleen CreditAttribution: bleen commentedthis 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
Comment #21
sunThanks!
Comment #22
marcvangend#20: 690544.patch queued for re-testing.
Just checking if the patch still applies.
Comment #23
bleen CreditAttribution: bleen commented#20: 690544.patch queued for re-testing.
Comment #24
aspilicious CreditAttribution: aspilicious commented#20: 690544.patch queued for re-testing.
Comment #26
bleen CreditAttribution: bleen commentedrerolled with fixed test
Comment #27
dbeheydt CreditAttribution: dbeheydt commentedrerolled to HEAD
Comment #29
andypost#26: 690544.patch queued for re-testing.
Comment #30
rschwab CreditAttribution: rschwab commentedOops I think you guys meant Bartik! Trying that in the test.
Comment #31
rschwab CreditAttribution: rschwab commentedClosed #890894: Menu registry should be flushed when switching themes as duplicate.
Comment #32
EvanDonovan CreditAttribution: EvanDonovan commentedWhitespace error.
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.
Comment #33
andypostRe-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
Comment #34
EvanDonovan CreditAttribution: EvanDonovan commentedOh, 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).
Should be
I promise I'll test the re-roll, although should be RTBC based on @sun's earlier review.
Comment #35
andypostDone
Comment #37
rschwab CreditAttribution: rschwab commented#35: 690544-rebuild-menu2.patch queued for re-testing.
Comment #38
robertom CreditAttribution: robertom commentedI 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
Comment #39
marcvangend#35: 690544-rebuild-menu2.patch queued for re-testing.
Comment #40
tumblingmug CreditAttribution: tumblingmug commentedThis 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.
Comment #41
marcvangendtumblingmug: 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.
Comment #42
joachim CreditAttribution: joachim commentedDon't fix indentation in this patch please :)
Powered by Dreditor.
Comment #43
robertom CreditAttribution: robertom commentedDone.
Re-rolled patch attached
Comment #44
robertom CreditAttribution: robertom commentedComment #45
robertom CreditAttribution: robertom commentedI'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
Comment #46
bleen CreditAttribution: bleen commentedrobertm: 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
Comment #47
tumblingmug CreditAttribution: tumblingmug commentedAt the end I agree with marcvangend (#41) and have tested the patch sucessfully.
Comment #48
rfaySorry. D8 first. Let's get these fixed on D8 and into D7.
Comment #49
bleen CreditAttribution: bleen commented#43: 690544-rebuild-menu3.patch queued for re-testing.
Comment #50
andypostBack to RTBC, well commented and test included
Comment #52
Dood59 CreditAttribution: Dood59 commentedThanks Robertom, it's could help !
Comment #53
catchtagging for backport.
Comment #54
tumblingmug CreditAttribution: tumblingmug commentedI 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.
Comment #55
catch@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.
Comment #56
tumblingmug CreditAttribution: tumblingmug commented@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!
Comment #57
catchI 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.
Comment #58
Dries CreditAttribution: Dries commentedGreat. Committed this patch to 7.x and 8.x. Thanks everyone for your participation.
Comment #59
Dries CreditAttribution: Dries commented