Summary as of November 2014
This problem has been fixed in D7 and D8.
See #43 for D7 repro steps, and #60, #61 and #62 confirming fixed in D7.
It is still a bug in D6.34.
The repro steps are in #40. Patch re-rolled in #68 with same code changes as in #40 but comment wrapping corrected.
Original starting post from February 2007, Drupal 5.1
I have upgraded from 5.0 to 5.1 and previously had several user-defined blocks with content and visibility set using php. They still function fine in 5.1 but when using admin -> block -> configure the actual php code is no longer shown in the text area. Likewise the title entry field is blank, even though the correct title is displayed in the real block. ie the data is stored OK in the db, but it is just not being shown in the configure entry form. I have proved I am looking at the same block by entering a new title, which then gets shown on the real block.
Ask if you want more info,
Jonathan
Comments
Comment #1
jonathan1055 CreditAttribution: jonathan1055 commentedSorry, the titles and meaning of the drop-down menus when submitting a bug are not very clear. This bug is nothing to do with the administration module (I have not even downloaded that one). I selected 'administration' thinking that 'project' meant the area of Drupal to which the bug relates. It can be moved to another project if more appropriate.
Jonathan
(previously known as mr_splosh - to aid references to this name in the thread)
Comment #2
dreed47 CreditAttribution: dreed47 commentedmoving to Drupal project
Comment #3
Anonymous (not verified) CreditAttribution: Anonymous commentedI noticed this same bug.
I took a look in the module code and this is what seems to be happening:
- As Mr_Splosh said, the data looks fine in the DB
- However, the settings for each block seem to be stored on a per theme basis (IE you can have the same block title for different themes)
- I noticed the code which pulls the data from the DB for the block config form (admin -> block -> configure) does not specify which theme to use in the data lookup
- This is the problem I think (at least it was in my case) because it was actually pulling the data from another theme's block (which was blank) rather than my current default theme
In my own code I just edited this file to pull from the block table using my current theme as a parameter (along with the module type and delta) and this seemed to fix this bug
Comment #4
joshk CreditAttribution: joshk commentedBumping and taking this on.
Comment #5
joshk CreditAttribution: joshk commentedOk, I have a working fix: basically we need to pass another arg in the url. I will post a patch shortly.
Comment #6
joshk CreditAttribution: joshk commentedHere's a patch that will fix this based on 5-dev. Shoud work for 5.3 installs too...
Comment #7
Gábor HojtsyHow come you can get rid of
isset($block['throttle']) ? $block['throttle'] : 0
in the code, just replacing it with$block['throttle']
?Comment #8
drummYes, changing that likely adds a slew of PHP notices when throttle module is off.
Comment #9
joshk CreditAttribution: joshk commentedGood catch. I did a full line-replacement from my existing install, so likely missed that update. I've gone back and re-rolled the patch from a fresh CVS checkout.
Comment #10
joshk CreditAttribution: joshk commentedAlso, fwiw this is already fixed for drupal 6, so no need to worry about it going fwd.
Comment #11
PasqualleThis works, thanks..
Comment #12
drummI having trouble reproducing the original issue. In Drupal 5, I am not sure if blocks should get custom settings per-theme. The navigation does suggest they might. However, any module-added settings are going to be set for every theme. Having every setting apply to every theme would be consistent.
Comment #13
PasqualleI spent more than an hour to reproduce this bug, it is not easy.
just copy paste the links to your address bar, do not need to navigate through the menu
1. clean drupal 5.5 install
2. admin/build/themes - click "Save configuration"
3. admin/build/block/list/garland - do nothing
4. admin/build/block/configure/user/3 - fill textfield "Pages:" with "anything" and click "Save block"
5. admin/build/block/list/bluemarine - do nothing
6. admin/build/block/add - fill textfield "Block description:" with "anything" and click "Save block"
7. admin/build/block/configure/user/3 - BUG The textfield "Pages:" is empty
So the problem is in point 5 that we got inconsistent rows in table {blocks} for
select pages from {blocks} where module = "user" and delta = 3;
the
pages
column contains "anything" for garland and empty for bluemarine, because blocks for bluemarine were initialized at this point with default values.The trick in point 6 that it somehow changes the order of rows in DB, so user/3 moves to the end of the table for garland theme.
and this code (which is used to display the block visibility) returns the first row found, which is now the empty bluemarine
The reproduction of the problem is not obvious, but it happens many times, if you add new blocks to your site.
I use this patch for a month now on live site without a problem.
And yes, you can have custom settings per-theme with this patch, but I like it. The design of {block} table suggest this behavior.
Comment #14
jefftrnr CreditAttribution: jefftrnr commentedI'm using Drupal 5.7 with several themes running via the project/domain module. This bug was a HUGE problem. So far, it seems solved with your block_visibility_theme.patch. This fix was crucial! Thanks so much... please be sure to have this patch included in the next Drupal 5 update!
Comment #15
drummCustom settings per-theme is not in Drupal 6. This bugfix must implement what is in Drupal 6, since the bug is fixed there. If it is done differently, this is a new feature and will cause configuration changes when you upgrade to Drupal 6.
Comment #16
PasqualleI don't understand you. In Drupal 6 you have custom settings for blocks per theme. Every theme has its own block visibility setting..
I did not checked the code, if it is same as this patch, but I know that the functionality is the same (or similar).
Comment #17
drummIf I click configure from http://drumm-laptop/6/admin/build/block, I go to:
http://drumm-laptop/6/admin/build/block/configure/user/1
If I click configure from http://drumm-laptop/6/admin/build/block/list/bluemarine, I go to:
http://drumm-laptop/6/admin/build/block/configure/user/1
These are the same, so how would Drupal 6 accomplish custom block settings per-theme?
Comment #18
Pasquallesh*t, you are right..
but this is so wrong. or is not? why we force same settings for every theme?
but, yes, the patch needs work, should be the same functionality as it is in drupal 6
Comment #19
jefftrnr CreditAttribution: jefftrnr commentedDoes anyone know how this block-settings-per-theme issue is accomplished in Drupal 6? I'd like to find out if this can be resolved in Drupal 5, either with this patch or some other. I'm now running drupal 5.10 and I don't think it's fixed yet.
Why doesn't this current patch work? Does it change the table schema that much? It seems to only make sure the correct columns are in the WHERE clauses. I'm not sure I follow drumm's concerns with this patch causing problems for the upgrade path to Drupal6. But, if I'm using the patch currently, will I have trouble upgrading the block db table when I want to use Drupal 6 later on?
Comment #20
Pasquallethis patch does not change the table schema. If you use this patch you may have to manually reset the visibility settings in all blocks, after you upgrade to Drupal 6 (but if you encounter this bug, then I think you have to do the D6 reset even if you do not use this patch, because the current D5 block visibility code is simply broken)..
the only "problem" with this patch is that it introduce a new "feature": Custom block visibility settings per-theme.. Such feature was never approved for Drupal core..
Comment #21
greenmachine CreditAttribution: greenmachine commentedThis problem has really only cropped up for me on a larger, more complex site (but not other sites of similar complexity). I have no idea why. My approach to fixing was a little different, didn't involve changing core code. Instead I built something that will warn when the rows representing one block (across multiple themes) have become desynched (title/visibility/pages no longer is the same for each theme). Here's the code:
Into hook_menu goes:
Into hook_form_alter goes:
Finally a new function for the blockfix form and a corresponding submit function:
Comment #22
jonathan1055 CreditAttribution: jonathan1055 commentedWow! It is nearly two years since I raised this bug, and it is still an ongoing problem.
Or rather, there is a similar problem but it is reversed. I now have a new site in D6.8 and when adding an existing block to a new theme, the block visibility is lost and the title is blank when browsing the site. Then, only after a 'configure' and then 'save' with no changes, does the title get shown and the visibility code enacted on. Is this the expected behaviour? ie every block must be edited and saved before the new theme operates correctly?
Comment #23
Pasqualle@Mr_Splosh: is it an upgraded D5 site, or you can reproduce the bug with a clean D6 install? If you can reproduce it with a clean D6 install, then please provide the reproduction steps.
Comment #24
robschmitt CreditAttribution: robschmitt commentedGotta say, it's pretty ridiculous that this issue has never been addressed in core. I've just run into this problem again -- and once again on a large site with dozens of blocks where I can ill afford my blocks randomly moving to different positions.
Comment #25
fmccormick CreditAttribution: fmccormick commentedHit this again on our latest project ... after hunting through the code it came down to the theme having been set before the block module is initialised (block module is setting a custom theme and if that doesn't get initialised properly
_regions isn't set which does bad things to your block table)
The culprit in our case was a module calling path_to_theme() in its menu - this calls init_theme() before block.module gets a chance to set the custom theme.
This would also rear its head if you do anything that requires a theme in the main module code (i.e. if it fires in the require_once)
We removed the offending path_to_theme() call which fixed our problem, however I've also implemented a security check which stops anything calling init_theme in the drupal_bootstrap segment.
The following code was added to init_theme() (in theme.inc) just after the globals:
Patch file attached for 5.15
Comment #26
fmccormick CreditAttribution: fmccormick commentedLooks like the above patch does bad things to redirects. I've modified it to only trigger if you're in admin/build/block or a subdirectory. Probably a better way to do that...
Comment #27
Nico_0 CreditAttribution: Nico_0 commentedI upgraded from 5.11 to 5.12 recently and now I am experiencing the same problems (title and visibility setting of block dissapearing).
What should I do?
Comment #28
PasqualleNico_O try the patch in #9 and read comment #20..
Comment #29
Nico_0 CreditAttribution: Nico_0 commentedThanks,
I applied the patch, and the visibility settings are showing again.
Comment #30
Tresler CreditAttribution: Tresler commentedAs far as I can tell, this isn't an issue in D6 and is beyond fixing in D5. Marking won't fix, and moving on. Re-open if there is more to add to this.
Comment #31
Pasquallewhat kind of reasoning is that? D5 is still supported, if someone creates an acceptable patch it still can be fixed..
Comment #32
jonathan1055 CreditAttribution: jonathan1055 commented@30
Hi, yes this is still an issue in D6. As far as I know it as not been fixed.
@31
I agree, that whilst D5 is still supported we should try to keep it running correctly.
Comment #33
Tresler CreditAttribution: Tresler commentedOk, apologies, thought this was a dead issue. My mistake.
Comment #34
landike CreditAttribution: landike commentedTHANKS PEOPLE !!!!!!!!!!!!
very helpful page..
Comment #35
jonathan1055 CreditAttribution: jonathan1055 commentedI've just run into this problem again and it is definitely not fixed as of D6.13 (unless Joshk from #10 can show me otherwise). I think we need to understand how D6 is intended to function before we can suggest fixes, and then port them back to D5. There is some urgency now, because D7 is coming up fast.
Does anyone know (or how can we find a difinitive answer) if custom theme settings per block is intended to be a D6 feature? The schema of the block module implies that it could be, otherwise there would be a separate table for some of the block settings and they would not be duplicated for every row in {blocks} just to add a different theme. However, the current functionality for block visibility by role uses the separate {block_roles} table which is linked by delta, which does allow separate settings per theme but often there is only one delta value across all themes, eg my search block has delta=0 for all themes, so whenever I change the visibility by role it changes for all themes. Other blocks have differing delta values which inplies that their visibility by role can differ across themes. So I can't tell what was actually intended, as it is a mixture of both.
Having custom block settings for different themes would certainly be a useful feature, for example, to have a 'standard' theme with most blocks displayed by default and a 'minimal' theme with most blocks not displayed but the user can turn them on if required. Or the search block in a 'standard' theme could have the title Search but in a 'Casual' theme it could be Go fetch this! It would certainly be quite easy to have this functionality, as it is almost there already! If this feature is intended then the code in the patch in #9 should be implemented. Also we could consider the excellent idea from Greenmachine in #21 which allows the admin to select from the existing differing settings - this could be very useful when adding a new block. The check from Fmccormick in #26 could also be added.
On the other hand, if custom settings per theme are not intended then we need to fix the code which initialises a new block when adding it to a theme so that the existing settings are copied to the new row in {blocks}. As it stands in D6, if you have configured a block in your default theme, then add it to a second theme the config settings are not copied forward, then when you save this new block, the settings are written over both the new and the old block rows, thus losing your previous configuration inputs.
I have applied the code changes in the patch in #9 to D6 (the functions have moved from block.module to block.admin.inc and a few $form keys needed to be changed, but the principle is identical and the base code has not changed at all from D5. I've attached the patch if anyone wants to try it out.
Jonathan
Comment #36
Pasqualleso, to answer your question. feature requests for D6 are not allowed. and also too late for D7.
So, "Custom block visibility settings per-theme" can be asked for Drupal 8 only, and please open a new issue, as this one should be for fixing the bug (found in D5).
I did not see this bug in D6, and the repro steps in #13 works correctly.. can you provide steps how to reproduce the bug in D6?
Comment #37
jonathan1055 CreditAttribution: jonathan1055 commentedHi,
I've got all the information prepared, with screen grabs and a patch, but strangely drupal.org is not allowing me to preview the post before submitting. The preview just returns my input textarea with my text, but no preview. It fails on other issue queues too. I'm not going to post without a preview, so it will have to wait! Also the file uploads get stalled at random places, sometimes after the the third file, sometimes after the second, so my post would be less useful without these. I posted Cant preview posts in issue queues to the general forum.
Comment #38
Pasqualleoff topic: the preview issue: #579900: Previewing comments is broken by FAPI change in Drupal core 6.14
Comment #39
landike CreditAttribution: landike commentedPeople.. JUST LET ME KNOW - this issue is fixed for Drupal 6.14 ??????
Will it be updated in Drupal 7?
Comment #40
jonathan1055 CreditAttribution: jonathan1055 commentedHi Pasqualle,
Yes I know that new features are not allowed, I was just trying to work out whether this was already intended as a feature which was not quite working correctly, given the comments in #13-19. OK we can take it that custom settings are not a feature and therefore we must fix D6 and D5 accordingly. There are two problems that have been discussed in this thread, and the following steps will illustrate both. Creating a custom block, then deleting it later is just a controlled way to simulate what can happen when modules which provide blocks are enabled, then disabled and uninstalled.
The reproduction steps are:
(the problem has now been created - the next two steps are just to illustrate it)
Explanation of the above steps with screen grabs of the {blocks} table:
We now have the two problems encountered at various stages in this thread. First, the block in Bluemarine does not contain the custom settings that were applied to the block in Garland. Unless and until the admin configures the block in Bluemarine, it will be shown on every bluemarine page and will not have the custom title. See step8 image showing the un-customised block on the 'my account' page versus the customised block in Garland (step9 image).
Secondly, if the admin attempts to configure the block, the data from the Bluemarine row is the one that gets extracted into the configuration form, not the row from the Garland theme that was customised in step 5. Thus there is no way to recover the previous customised settings. As soon as the block configuration form is saved then the new data will be written over both the rows in the blocks table, thus erasing the original data completely.
A simple solution for the second problem in to order the sql select by block id. Only the first row is returned, and currently there is no ORDER BY, so the first matching row in the table is used (which in this example is the empty row inserted between bid 9 and 12). With the select ordered by block id then the older row with lower block id (and with the customisations) would be returned. This allows the admin to recover the original customisations, which when saved again will be propagated to all rows for that block, across the other themes. The attached patch _block_settings.order_by_bid does this, which makes the change in block_admin_configure() within block.admin.inc
For the first problem, to give a new block the existing customised settings and avoid it only getting the defaults, I suggest a change to _block_rehash() in block.module. This is where new rows are added to {blocks} when they are missing for a specific theme. The attached patch _block_settings.existing_block_values retrieves an existing row for this block and uses the data for the new row.
I have not checked whether these steps and the fixes are the same for D5, but the code base is very similar. I will leave that until after my suggestions have been reviewed and we agreed about the correct method for fixing D6.
Jonathan
Comment #41
PasqualleFrom the repro steps it seems it is a bug in D6 also.
My first question would be: is it a bug in D7? because everything should be fixed in the latest version first and backported. The solution would be properly reviewed. There were some block changes in D7, so I do not know..
The "order by bid" solution seems like a quick fix which could still break sometime, but I am not sure if I could create a valid case when it would break. Like if I would completely uninstall a theme, with removing the records in the blocks table manually (or with a special contrib module), then I would lose the block settings which are not yet propagated to other themes.
my personal opinion: if it is a bug in D7 then maybe it would be good, to still ask for the feature request in D7 as it could be a much cleaner solution..
Comment #42
jonathan1055 CreditAttribution: jonathan1055 commentedYes I get your point about checking D7 first. I will try to look into that.
The simple fix with 'order by' is only to help those who currently have mis-matched rows. The main fix will stop this error happening in future, so they are both required for D6.
Comment #43
jonathan1055 CreditAttribution: jonathan1055 commentedYes, I've tested this on D7 and the problem exists there too. Some work has been done in block.admin which does improve things though. Here are the repro steps:
(b) Configure 'Recent Blog Posts', set title = 'Custom title for blog', set visibility to only show on <front> page, set user specific 'show but allow to hide'
After the custom configuration is saved and bid=25 is updated, a new row is added for Stark, with bid=26. This has the 'pages' value copied, but 'custom', 'visibility' and 'title' not copied, which gives us a mis-match across themes. See image step 5b. If the blocks are now assigned to regions [and a blog content post is written ] then in Garland the block is displayed on the front page with the customised title, but in Stark it is shown on every page except the front page and it only has the default title. The last two images show this.
A fix is required to the code in block_admin_configure_submit() in block.admin.inc, which creates new rows for the other enabled themes.
I have made a patch for this, and changed the version to 7.x-dev.
Jonathan
[edit on 23.09.2010 only for fixing a spelling mistake]
Comment #44
jonathan1055 CreditAttribution: jonathan1055 commentedThought I'd try uploading the same patch for automated testing. Not tried this before.
Comment #45
PasqualleI think we will need a simpletest for this one, but it looks like it will be quite complicated. Any simpletest guru here?
Comment #46
jonathan1055 CreditAttribution: jonathan1055 commentedThe D7 patch was subjected to 427 Block tests and passed them all. Attached is the results after applying the patch and re-running repro steps 1-5.
Jonathan
Comment #47
PasqualleOk, but those 427 tests pass even without the patch.. We need at least one test which would fail without the patch, so we won't reintroduce this bug in the future..
Comment #48
cburschkaComments should wrap after 80 characters.
Also, needs test case.
I'm on crack. Are you, too?
Comment #49
sunAnyone up to re-roll this patch?
Comment #50
exaboy CreditAttribution: exaboy commentedI need this patch for D6.17...
Comment #51
jonathan1055 CreditAttribution: jonathan1055 commentedHi,
The patch I gave in #35 still applies OK in 6.16, and I've just checked and block.admin.inc is identical from 6.16 to 6.17 so you can use it.
Jonathan
Comment #52
ohnobinki CreditAttribution: ohnobinki commented+1
Comment #53
ohnobinki CreditAttribution: ohnobinki commentedThe requested re-roll against HEAD.
Comment #54
mrfelton CreditAttribution: mrfelton commentedWith the patch at #35 applied to D6, block multilingual settings do not save properly.
Comment #55
mrfelton CreditAttribution: mrfelton commentedIgnore my previous comment - it's unrelated, and due to the i18n issue in #766678: The "All languages" option can't save back from the block Multilingual settings.
Comment #56
jonathan1055 CreditAttribution: jonathan1055 commented#53: 115596.block_admin.copy_existing_settings-D7.patch queued for re-testing.
Just checking the latest status. Would be good to get this accepted, then we can return to work on the D6 patch.
Comment #57
mcarbone CreditAttribution: mcarbone commentedComment #58
xjmThis no longer applies.
Comment #59
gpk CreditAttribution: gpk commentedIs this a problem in 8.x still? Have certainly seen it in earlier versions tho'...
(also reworded title slightly)
Comment #60
jonathan1055 CreditAttribution: jonathan1055 commentedI think it might be fixed in D8 and D7 actually. I have just tried my repro steps from #43 in D7.12 and everything appeared to work ok. I have got screen grabs of the block table at each point, see below. However, I want to go back and compare the source code at various releases to see exactly when it was fixed, as there is nothing in this thread which says anything about a commit to D7. It must have been done in a different issue.
If it is fixed then we can port the correction back to D6 to fix the bugs there.
Comment #61
xjmHmm, other than the fix to transaction handling in #1022416: Wrong use of db_transaction() in block module, these lines haven't changed since #337212: DBTNG: Block module, so if it was fixed, it was not fixed in this way.
So, let's get one other person confirming this isn't reproducible in D7 and, if so, bump it back to D6?
Comment #62
cossovich CreditAttribution: cossovich commentedI can't reproduce this bug on D7 (using a fresh copy of 7.12) following the steps at #43.
The difference is that at step 5a I get 2 new rows for the block table, bid=25 for bartik and bid=26 for stark. After the custom configuration step at 5b (customising title and visibility settings for bartik theme) the custom options are also saved for stark in the bid=26 row.
Seems as though this is fixed for D7.
Comment #63
xjmThanks @cossovich! Based on #60 and #62, moving this issue back to D6.
Comment #64
jonathan1055 CreditAttribution: jonathan1055 commentedMy two patches in #40 are (probably) still valid as an easy fix and simple to understand. May need a re-roll though.
Comment #65
jonathan1055 CreditAttribution: jonathan1055 commentedHere's a re-rolled patch - same code changes as in #40 but comments now wrap at 80. In some set-ups the problem is slightly mitigated by newer database versions which do not add new rows in the space vacated by deleted rows. So if you follow the repro steps in #40 you may find the second problem is eased, where you can recover from the lost settings. But the main problem is still there - that blocks lose their custom title, visibility and pages values when a new theme is enabled.
I've also written an additional simpletest case in the /simpletest/tests/block.test file, if anyone would like to see the problem demonstrated automatically.
Comment #68
jonathan1055 CreditAttribution: jonathan1055 commented