This is a followup to #410646: "Secondary menu" exists but is no longer the default source for the secondary links. Now that that is in, the hardcoded Secondary Menu is gone from core (yay).

As per discussion there, we should add a Footer Menu to the standard install profile and enable its associated block to appear in the footer region of Bartik by default.

That way, if you want to add standard "footer links" to your site, you just add them to this menu and they automatically display in the right place - similar to the way you can currently add main navigation links to the Main Menu and they automatically display in the right place.

Files: 
CommentFileSizeAuthor
#30 platform.footer.30.patch9.19 KBsun
PASSED: [[SimpleTest]]: [MySQL] 48,137 pass(es).
[ View ]
#21 interdiff.txt5.16 KBsun
#21 platform.footer.21.patch9.63 KBsun
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch platform.footer.21.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#14 drupal8.footer-menu.14.patch6.32 KBsun
PASSED: [[SimpleTest]]: [MySQL] 46,324 pass(es).
[ View ]
#13 drupal8.footer-menu.13.patch6.3 KBsun
PASSED: [[SimpleTest]]: [MySQL] 46,332 pass(es).
[ View ]
#11 drupal8.footer-menu.11.patch3.49 KBsun
PASSED: [[SimpleTest]]: [MySQL] 46,312 pass(es).
[ View ]
#4 drupal8.footer-menu.4.patch2.22 KBsun
PASSED: [[SimpleTest]]: [MySQL] 46,254 pass(es).
[ View ]
#2 891670.patch3.02 KBbleen18
PASSED: [[SimpleTest]]: [MySQL] 35,057 pass(es).
[ View ]
#2 Welcome to Drupal | Drupal.png58.01 KBbleen18

Comments

Tor Arne Thune’s picture

Version:7.x-dev» 8.x-dev

Still a valid issue in Drupal 7.4. I'm not sure if this goes under string freeze or not, so not adding backport to D7 tag.

bleen18’s picture

Component:install system» menu system
Status:Active» Needs review
StatusFileSize
new58.01 KB
new3.02 KB
PASSED: [[SimpleTest]]: [MySQL] 35,057 pass(es).
[ View ]

This patch adds a new menu called "Footer Menu" and puts it in Bartik's footer region. I wasnt sure what default link(s) to put in there, so for now I just put a "home" link, but that is something to discuss. Also, I tweaked a function in Bartik's template.php to hide titles from blocks in the footer region like we do already in the header region. I think that the same assumption is valid.

sun’s picture

Title:Add a Footer Menu to the standard install profile» Add a Footer menu to the built-in system menus
Issue tags:+Usability, +Platform Initiative

#594660: Rename default menu names just landed.

We want to rename the internal menu name to just 'footer'.

There's a follow-up patch required in aforementioned issue though, which will prefix the system block identifiers with 'menu-'.

sun’s picture

StatusFileSize
new2.22 KB
PASSED: [[SimpleTest]]: [MySQL] 46,254 pass(es).
[ View ]

Re-rolled against HEAD and simplified interface texts.

Shyamala’s picture

Status:Needs review» Needs work

Applying patch throws up the below error

error: patch failed: core/includes/menu.inc:1755
error: core/includes/menu.inc: patch does not apply
error: patch failed: core/modules/menu/menu.install:51
error: core/modules/menu/menu.install: patch does not apply
sun’s picture

Status:Needs work» Needs review

#4: drupal8.footer-menu.4.patch queued for re-testing.

sun’s picture

@Shyamala: That's odd, sent the patch for re-testing, to prove what you're saying. In any case, please also double-check yourself that your local 8.x checkout is up to date.

sun’s picture

Assigned:Unassigned» sun
Shyamala’s picture

The patch applys. No error occurs, code updates well.

  • Applied patch and cleared cache, but the Footer menu did not apply.
  • Checked in admin/structure/menu, menu not listed.
  • Check in Bartik setting page, there is no option for Footer menu.

Is there anything else I need to do?

Eric_A’s picture

@Shyamala, have you run all database updates? A particularly interesting one is from this commit: http://drupalcode.org/project/drupal.git/commit/3666987

sun’s picture

StatusFileSize
new3.49 KB
PASSED: [[SimpleTest]]: [MySQL] 46,312 pass(es).
[ View ]

So I thought about what would be a good use-case for a footer menu link in core, and came up with:

"Contact" :)

Attached patch adjusts Contact module accordingly, enables Contact module for the Standard installation profile, and also adds a database update for Menu module to inject the new menu into custom menus on existing sites (which is required to make it editable in the UI like all other menus).

sun’s picture

That said,

+++ b/core/modules/contact/contact.module
@@ -90,7 +90,7 @@ function contact_menu() {
     'type' => MENU_SUGGESTED_ITEM,

Contact module exposes the "Contact" menu link as a suggested item only. This means that the link is initially disabled.

We could change this item into a normal item, but that would get us into quite some scope-creep.

Instead, we adjust the Standard profile to enable it during installation.

sun’s picture

StatusFileSize
new6.3 KB
PASSED: [[SimpleTest]]: [MySQL] 46,332 pass(es).
[ View ]

Incorporated the suggestion from #12; i.e., Standard profile enables the Contact link in the Footer menu.

Apparently, the menu system does not provide a facility to do that yet, so I quickly had to invent one.

sun’s picture

StatusFileSize
new6.32 KB
PASSED: [[SimpleTest]]: [MySQL] 46,324 pass(es).
[ View ]

Small adjustment to the menu update.

bleen18’s picture

+++ b/core/includes/menu.incundefined
@@ -3345,15 +3346,32 @@ function menu_link_maintain($module, $op, $link_path, $link_title) {
+      $result = db_query("SELECT * FROM {menu_links} WHERE link_path = :link_path AND module = :module AND customized = 0", array(':link_path' => $link_path, ':module' => $module))->fetchAll(PDO::FETCH_ASSOC);

why doesn't this use the DBTNG syntax? ... I'm sure there is some coding standard that I'm just not familiar with, but I thought I'd ask just in case

Otherwise I love this patch ... applied cleanly and after a fresh install I had a contact link in my shiny new footer menu. RTBC from me

sun’s picture

The code is running a targeted query against the menu link table, which does not need the db_select() syntax. Also, the added query is using exactly the same approach as the existing $op 'update'. If that was bogus, then it would have to be changed in a separate issue anyway. :)

So I'm counting one RTBC here. Any further? :) Perhaps even an issue status change? ;)

barraponto’s picture

Just installed using drush si and footer menu was there at the footer, but contact link was disabled.

barraponto’s picture

Status:Needs review» Needs work

I guess that's "Needs Work"

sun’s picture

Status:Needs work» Needs review

@barraponto: I'm relatively confident that Drush is not up to speed with regard to D8 at this point. At least, it does not work (more or less at all) for me.

Also, your review directly contradicts #15, which says that the link appeared just fine after performing a manual installation using Standard profile. Can you double-check? :)

barraponto’s picture

Status:Needs review» Needs work

Ok, I applied the patch from #14 (still applies cleanly) and manually installed using standard profile.
Still shows footer menu in the footer region, leaves contact link disabled :/

FWIW, I'm running PHP 5.4.8, MySQL 5.5.28 and Lighttpd 1.4.31. I'm using php-fpm (shouldn't matter).

Hmmm, is menu.inc loaded by the time standard_install runs?

sun’s picture

Status:Needs work» Needs review
StatusFileSize
new9.63 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch platform.footer.21.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new5.16 KB

Alright, sorry, confirmed your manual testing results, and fixed them:

  1. Fixed Simpletest does not support tests for installation profiles.
  2. Added Standard installation profile test.
  3. Fixed missing user access to contact form after installation.
  4. Fixed menu link has to be marked as customized after toggling hidden.
  5. Removed entirely needless menu_router_rebuild() from standard.install.

Now the expectation is covered by automated tests. This is the first test for an installation profile in core, since testing of installation profiles was not possible before.

sun’s picture

Alrighty:

Standard installation profile   7 passes, 0 fails, and 0 exceptions

Are we done? :)

barraponto’s picture

Confirm menu contact link is there after installation, even for anonymous users. Are we sure we want to leave this open by default, for possible abuse by spammer bots?

btw, I didn't read the installation profile test stuff (don't know simpletest). leaving it "needs review" for someone else to read it. but good work sun :)

Shyamala’s picture

Status:Needs review» Reviewed & tested by the community

Voila! It's works awesome.

  1. Downloaded the latest version of Drupal 8
  2. Applied the patch
  3. Installed using Standard Profile

The contact link on the footer showed up!

webchick’s picture

Category:task» feature

This sounds like a good idea, but it's a new feature, and thus subject to thresholds, which we are over atm. :(

catch’s picture

Why all the changes to menu_link_maintain() and not just menu_link_save() in the install profile?

sun’s picture

Contact module registers a "Contact" link already, which we are putting in the footer menu by default.

That's a suggested item though and thus not enabled by default. We do not want to create a duplicate Contact link in that menu.

In order to not create/save a duplicate menu link, the existing menu link needs to be known and loaded, and it must be passed along to menu_link_save(), so that the existing mlid and plid is known and the existing link can be updated. The menu [link] system does not provide API functions for that.

However, the menu link system provides an API function that generally has the exact purpose of allowing code (not humans) to maintain a menu link: menu_link_maintain()

The important detail about it is that it only touches a menu link if it hasn't been customized. So this is exactly the API function that is needed, but it didn't support enable/disable operations yet. This patch here adds that tweak, so Standard profile can enable the default "Contact" link in a one-liner.

sun’s picture

#21: platform.footer.21.patch queued for re-testing.

Status:Reviewed & tested by the community» Needs work
Issue tags:+Usability, +Platform Initiative

The last submitted patch, platform.footer.21.patch, failed testing.

sun’s picture

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new9.19 KB
PASSED: [[SimpleTest]]: [MySQL] 48,137 pass(es).
[ View ]

Merged HEAD.

sun’s picture

#30: platform.footer.30.patch queued for re-testing.

catch’s picture

Title:Add a Footer menu to the built-in system menus» Change notice: Add a Footer menu to the built-in system menus
Category:feature» task
Priority:Normal» Critical
Status:Reviewed & tested by the community» Active

I don't really like suggested items or menu_link_maintain() very much, but this patch isn't responsible for either of those so it's not really fair to hold it up on it.

I do think a new menu makes some sense and contact is a good use-case for it. I could see legal module adding a link there by default too.

Committed/pushed to 8.x. Could use a change notice.

sun’s picture

Title:Change notice: Add a Footer menu to the built-in system menus» Add a Footer menu to the built-in system menus
Category:task» feature
Priority:Critical» Normal
Status:Active» Fixed

Thank you! :)

Change notice: http://drupal.org/node/1845756

Status:Fixed» Closed (fixed)

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

mr.baileys’s picture

Follow-up: Contact module still has some help text referring to the Contact-link being in the Tools-menu: #1873416: Contact module help text incorrect.