Posted by Gábor Hojtsy on May 4, 2009 at 12:06pm
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | theme system |
| Category: | task |
| Priority: | normal |
| Assigned: | Gábor Hojtsy |
| Status: | closed (fixed) |
| Issue tags: | Favorite-of-Dries |
Issue Summary
Along the lines of #240873: Move custom help settings to blocks, #428744: Make the main page content a real block and especially #428800: Convert mission statements to a region with blocks, here is a proposal to remove the footer message and make it a custom block instead. The footer message is one of the ancient items in Drupal which date back to times before the flexible regions system. Now it is not a problem to put blocks at the bottom of the content region or to the footer specifically. Thus having a dedicated footer message seems to be special casing something which we can empower with text format support, free placement, page visibility settings, caching and so on. Way better.
Upgrade path yet untested.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| remove-footer-message.patch | 7.98 KB | Idle | Failed: Failed to apply patch. | View details |
Comments
#1
Yes, puh-lease. This patch looks good. Will commit unless someone has a concern with it.
#2
On further review, the drupal_set_message() has a bug, where it says $bid_max + 2, it should be $bid_max + 3 instead, just like how the INSERT before it already uses that. Otherwise still looks good to me :) Fixed patch attached.
#3
No complaints here. A question would be what happens if this upgrade path runs on a theme without a 'footer' region? For example, Damien informs me that in French, 'footer' is 'pied de page' so I could see custom themes for international sites using region names like this.
#4
@webchick: the theme can still have 'footer' as the internal name for the region and have a nice name for the user, so it could still have a 'footer' region displayed with a different name.
Also, while going through themes, we could technically go through all regions of the theme at hand, and look at whether it has a footer region. Then if it does not have that, we can do something else. If we do not enable the block in that case, the user will not get a call to action to fix it. Now if the blocks page finds a block put into a region, which does not exist, it immediately prints the error and lets you reposition the block.
In the latest mission statement patch, I have this text at the end:
'If your theme does not have a highlighted content region, you might need to <a href="' . url('admin/build/block') . '">relocate the block</a>.'We can obviously adapt that and add that in. I think we cannot overcome manual work on the administrators part if there is no region named 'footer', but the best we can do is to attempt to put the block into that region, because then the user is alerted about the misconfiguration. Otherwise the block would be just "lost" among the possibly long list of disabled blocks.
#5
Given that this a major upgrade, some manual work will be inevitable. The key is that we don't loose people's data -- so I'd rather create an extra block that needs to be repositioned, than loose someone's footer message.
#6
#7
In the case of no footer region, how about putting it together with $content region and prompt user a message?
#8
Added requested language to the message in the patch:
'If your theme does not have a footer region, you might need to <a href="' . url('admin/build/block') . '">relocate the block</a>.'IMHO it is good to go.
#9
I looked the patch. It's working, but I see the following message:
patching file modules/system/system.install
Hunk #1 succeeded at 3348 (offset 61 lines).
I don't know it is important or I can set the status to RTBC.
pp
#10
Patch looks good, that offset doesn't matter. I didn't test the upgrade path though. If someone has then this looks ready.
#11
Actually upgrading won't work.
Immediately after the upgrade I get following messages:
The footer message has been successfully turned into the block (as I have been notified on update.php) but it has not been placed in the footer region (contrary to what update.php said). Instead I get the error message above and my footer reads "N/A". When I go to amdin/build/block and place the block called "Footer message" in the Footer region, everythings fine.
UPDATE: Maybe that will help debugging:
I looked in my database and the footer block actually reaches my block table and is enabled in the footer region (from looking into the DB, on the site it isn't, see above). When I drag the block into the footer region where it should be, the only thing that changes in that column of the block table is:
delta: before: 3, now: 1
bid: before: 5, now: 6
(and the weight from -10 to -8 but I suppose that's due to the drag and drop resetting the weights)
#12
Right, there are some issues with the $bid counting, which prevents the upgrade path from working right. I went in and fixed the issues in http://drupal.org/node/428800#comment-1574840 (in the site mission migration patch). Marking this postponed on that one, since I am not going to have the same fixes multiplied :)
#13
Now that the mission statement patch landed, I've rerolled and tested this one. Since there is no new functionality, only the upgrade path was tested. I've set both mission statement and footer and run testing on the upgrade. It works. Some screenshots to illustrate the changes:
Messages after update:
Blocks after update:
Site information settings on Drupal 7 with patch:
#14
Looking at this last screenshot, I realized the anonymous user settings should not be at the site information settings at all. Should be moved to the user settings. Patch solving that at #460296: Clean up user settings form.
#15
Looks good, and thanks for the extensive testing. Committed to CVS HEAD! Awesome patch.
#16
Great, documented the change at the themers upgrade guide:
http://drupal.org/node/254940#footer-message
#17
Automatically closed -- issue fixed for 2 weeks with no activity.