[UX] Have a "Contact" menu item be enabled and added to the primary navigation by default when enabling contact.module.

[klonos]

Part of #1168

Chances are people will want this menu item enabled by default.

Related: #1571

PR: https://github.com/backdrop/backdrop/pull/2145

[klonos]

...also adjust its weight so that it comes after the "Home" menu item.

[quicksketch]

:+1:

[opi]

I'm trying to work on this issue, but I can't find this disabled contact menu item... am I missing something ? @klonos @quicksketch

[klonos]

Yes, something must have changed between then and now. Now there is no "Contact" menu item in the first place (enabled or disabled). Retitling...

[opi]

Create a WIP PR https://github.com/backdrop/backdrop/pull/2145

Add my own review with some point I want to discuss https://github.com/backdrop/backdrop/pull/2145#pullrequestreview-114667190

[bugfolder]

There are unresolved comments in the PR (and unanswered questions), so moving this back to Needs Work.

[stpaultim]

I've created a new PR based almost exclusively on the work of @opi. I've made minor changes based upon feedback in the old PR.

[izmeez]

I just tested the PR and I think it needs work. Left details with the PR.

[stpaultim]

Feedback in PR comments:

I just tested the PR and I think it needs work. If the Contact module is enabled everything works fine. If it is disabled, not uninstalled and enabled again there is no Contact menu item and nothing to uninstall.

@izmeez Thanks for testing and providing comments. However, after more testing myself (on local environment), I think that this may work as expected. But, you have raised a good question and I welcome additional feedback.

The "Contact" menu link is generated on installation. So, if the module is disabled and re-enabled, it does not get created the second time, because (I believe) the install hook is not called the second time. The link to the form works and one is able to recreate the link manually.

If one disabled the module again, then it is available for uninstall. Once uninstalled and reinstalled, it does create the menu link again.

In my opinion, the menu link is not critical and many people may move or remove the link anyway. We create the link the first time the module is installed, to help people understand that the module is working and where to find the form. If they disable and then re-enable the module, I think it's OK that the link is not recreated, because hopefully by then they understand how the module works and create the link themselves.

Also, they have the option to disable, uninstall, and then reinstall the module to reset things.

Does this logic make sense to others?

[klonos]

I'm reviewing/testing this one now 👀

[klonos]

Here's a suggestion of how I think things should ideally work (not 100% sure if everything I'm suggesting is possible though):

• Each time the Contact module is enabled (not only for the first time - in general), it should check the following things:
  • Is the "Primary navigation" menu still available on the site, or has it been deleted?
    • If deleted, then do nothing.
    • If the "Primary navigation" menu exists, then check if there exists a menu item within it that points to /contact.
    • If there is, then:
      • throw a message to let people know
      • if the /contact menu item is disabled, let people know, and suggest that they might want to consider enabling it and moving it where they want etc. + also provide a link within the message that points to the "Manage links" page for the "Primary navigation" menu
      • if the menu item is already enabled, simply let people know that and do nothing else
    • If there is no /contact menu item in the "Primary navigation" menu, then:
      • add one
      • throw a message to let people know that that was done for them
      • add a couple of useful links to the Contact module settings page and the link management page for the "Primary navigation" menu (where the /contact menu item has just been added).
• When the Contact module is being disabled, check to see if a /contact menu item exists in the "Primary navigation" menu. If it doesn't, then do nothing. If it does, then warn that it might not work any longer, and offer an option to disable it (as opposed to delete it). This allows people to keep the menu item in place if for instance they are planning to add another page/view/webform/whatever there.
• When the Contact module is being uninstalled, do nothing further (the assumption being that before being able to uninstall it, the module must have been disabled first - if that has already happened, then we should have already done what needs to be done - see previous bullet points).

In general, I don't think that it's a good idea to go deleting people's menu items, in case they are planning to use them for something other than what the Contact module offers.

Does the above sound like a reasonable UX, with as few WTFs as possible to others?

[klonos]

Is the "Primary navigation" menu still available on the site, or has it been deleted?

Well, the primary navigation menu, the admin menu, and the user account menu are "system" menus (defined in menu_list_system_menus()), and system menus cannot be deleted apparently (not from the admin UI at least - the "Delete" operation is not available).

That's one less check to perform I guess.

[klonos]

...turns out that there is a way to achieve what this issue here is requesting with a very simple, one-line change. It is an in-built feature that the Menu module provides and makes available via the hook_menu() hook.

Here's a PR that simply adds that one-line change in the contact_menu() hook implementation: https://github.com/backdrop/backdrop/pull/4723 ... it may not do everything as expected though (referring to all the logic I've outlined in my previous-to-last comment above). But it handles automagically the addition/removal of the link to/from the Primary navigation menu upon installation/uninstallation of the module. The menu item gets added in a disabled state though, which we can either notify people about so that they can go enable it, or add some logic to auto-enable it and position it.

[stpaultim]

@klonos - Thanks. I has simply revived an old PR so was not invested in that solution. While, your one line solution is an improvement, for sure, it doesn't meet the original "acceptance criteria" in the title of the issue. This issue specifically requests that the menu item be "added" AND "enabled."

To quote the (very smart) author of this issue "Chances are people will want this menu item enabled by default."

I agree that a status message might be good enough. But, better would be to figure out how to enable it as effeciently as possible. The old PR did that.

I also noticed something in testing this module, which might be considered a bug - although I don't see any damage being done.

The first time you enable the module, the "Contact" menu item is added in a disabled state (as we discussed already). But, if you disable AND uninstall the module and then re-enable it. The "Contact" menu item is now added and enabled. This only concerns me, because it suggests that that the "uninstall" process is not fully removing the configuration. Does this seem like a problem? Could this have other negative implications?

[klonos]

While, your one line solution is an improvement, for sure, it doesn't meet the original "acceptance criteria" in the title of the issue. This issue specifically requests that the menu item be "added" AND "enabled."

Yup, didn't get to that yet. Just wanted to point out that there's an easier and more standard/proper way to achieve parts of what we have been trying to do with more custom code/logic.

I also noticed something in testing this module, which might be considered a bug ...

I'll have a look at that too 👍🏼

Also, after our discussion during the office hours, I'm switching this to a task rather than a new feature.

[stpaultim]

New PR - slight variation on PR by @klonos - https://github.com/backdrop/backdrop/pull/4735

This time the link is automatically enabled. Only one new line of code and one small change to another line of code.

@klonos, @izmeez OR @bugfolder - Have time to test??

[izmeez]

The PR works. It is a surprisingly brief one. Downloaded diff and tested on 1.28.0-preview. Thank you.

[izmeez]

Any chance this will get into 1.28.0 release? I'm not sure if it will cause problems for sites that have enabled the Contact module and placed the menu item somewhere else.

[stpaultim]

@izmeez You are correct, this is not backward compatible. I tested this on a site by first enabling the contact module, creating a specifical menu with a contact link and placing that new menu on the front page in a block.

I then edited the contact module to include this change and did not effect the previously placed link, but added a new link into the main menu. This would be an unexpected change on a live site and this can't work.

The original PR was a lot more lines of code, but it made changes during the module install process, so it would be backward compatible, and only make changes on initial installation of the module.

I'm wondering if it's possible to make the kind of change that @klonos recommended and still provide a link, without effecting existing sites in adverse ways.

[stpaultim]

I'm going to go back to this PR - https://github.com/backdrop/backdrop/pull/4697 - since the simplier solution seems vulnerable to backwards compatibility issues.

The only known issue with this PR is that after enabling and disabling the module, if one reenables it again, the link is not created. I don't believe that should be an issue.

@klonos had advocated for a simplier solution that creates the link, but does not enable. I believe the point of this issue was to enable the link to make the contact page visible and easy to find. Much as we do when new pages are created and a menu item is automatically created.

@klonos has suggested some advanced logic here (https://github.com/backdrop/backdrop-issues/issues/1572#issuecomment-2088270118). I feel like this might be over-thinking the problem, but I'm open to additional feedback.

I took on this PR, because I thought it might be any easy to get an existing PR across the finish line. But, I don't feel strongly about it. If there is no interest in this very minor improvement, I'll move on to other things.

[klonos]

@stpaultim in the now-closed PR https://github.com/backdrop/backdrop/pull/4735/files you have changed the type of the menu from MENU_SUGGESTED_ITEM to MENU_NORMAL_ITEM. I haven't tested yet, but I'm thinking that could be the cause of the issues that you are describing above.

Please refer to the following:

• https://docs.backdropcms.org/api/backdrop/core%21includes%21menu.inc/constant/MENU_SUGGESTED_ITEM/1
• https://docs.backdropcms.org/api/backdrop/core%21includes%21menu.inc/constant/MENU_NORMAL_ITEM/1

When I was testing locally with my PR (which doesn't change the type of the menu - it only adds the optional menu_name property to the menu item in the hook_menu() implementation), I think that I didn't see any such issues. I'll need to test again though, to be 100% sure.

[stpaultim]

@klonos I'm certain the use of MEN_NORMAL_ITEM caused my backwards compatibility problem. No need to test that. While your PR with MENU_SUGGESTED_ITEM is backward compatible and much simplier, it does not meet the goal of this issue, which is an enabled menu link for the Contact Form.

I guess, I just have no idea of how to enable the menu link in a backward compatible way without using the install hook, so I'm reverting to that solution.

I don't think you have identified a specific problem with the 36 lines of code my PR uses (originally from @Opi). You have just said that there MIGHT be a better or simpler way to do this. It seems that this PR is currently blocked, because of a possible (but unidentified) better way to accomplish the task.

I'll put this PR on hold, pending additional feedback or a better idea.

[indigoxela]

BTW, we have a related issue how to deal with MENU_SUGGESTED_ITEM: https://github.com/backdrop/backdrop-issues/issues/5674

[jenlampton]

I like the idea of adding a menu item by default. I would like to see a few changes in the PR

• On disable: disable the menu item (do not delete)
• On enable: re-enable the menu item
• On uninstall: delete the menu item
• On install: create the menu item
• We will need tests for all of the above :D

Needing tests does make this a feature request and not a task, so I've also added this as a milestone candidate.

[indigoxela]

@jenlampton I don't think, we should make "contact" a special case. The same problem applies to search and books. We should fix them together. Again the other issue re MENU_SUGGESTED_ITEM: https://github.com/backdrop/backdrop-issues/issues/5674 Current handling of MENU_SUGGESTED_ITEM is the base problem.

[stpaultim]

I don't think, we should make "contact" a special case. The same problem applies to search and books. We should fix them together.

@indigoxela - It's not at all obvious to me how this is related to the situation for search and books. Can you elaborate a bit? I don't recall that being mentioned before.

[quicksketch]

5674 is about how MENU_SUGGESTED_ITEM items get lost on a D7 upgrade.

I think the more relevant issue might be #5834 (either get rid of MENU_SUGGESTED_ITEM or change the way it works).

The crux of the issue here is that MENU_SUGGESTED_ITEM don't work in Backdrop, and even if they did, the entire implementation in D7 was weird. I really don't like how any menu item that was a MENU_SUGGESTED_ITEM could not be deleted, so you'd just end up with the suggested item in your menu forever. It could be disabled, but short of using a hook_menu_alter(), suggested items were forever in the UI. While we have similar concepts elsewhere (such as default Image Styles or default Views), it feels different for menu items (let me know if you don't agree). Perhaps because menu items are effectively content, compared to the configuration of those other items (and they can be removed forcibly by deleting their config files).

So what's being proposed here might be "the way we wish MENU_SUGGESTED_ITEM worked". We could potentially make MENU_SUGGESTED_ITEM work as described here. Which would mean the following:

• Contact module would simply specify /contact as MENU_SUGGESTED_ITEM.
• In #5834, we would make it so that:
  • On module install, MENU_SUGGESTED_ITEM menu items would get inserted into the specified menu from hook_menu().
  • On module disable MENU_SUGGESTED_ITEM menu items would get disabled.
  • On module enable MENU_SUGGESTED_ITEM menu items would be enabled.
  • On module uninstall MENU_SUGGESTED_ITEM menu items would be deleted.
  • Unlike D7, we would allow the menu item to be deleted. In which case it should stay deleted when disabling/enabling the module. But re-installing the module would recreate the menu item.

But if we wanted to just make contact module do this operation without doing #5834, we could do all of that manually in this issue, then remove it #4834. Doing it manually in this issue would leave the door open to completely removing MENU_SUGGESTED_ITEM rather than trying to fix it.

[indigoxela]

Doing it manually in this issue would leave the door open to completely removing MENU_SUGGESTED_ITEM rather than trying to fix it.

Fair enough. The concept of MENU_SUGGESTED_ITEM is really odd. :wink: But could we completely remove it before 2.x? D7 upgrades also need consideration.