It is possible to add invalid menu items to the top_menu setting. If an invalid item is the first item in the SiteSetting.top_menu list, the site will broken in a way that can’t be fixed through the UI.
I’m not sure if there’s ever a reason to be able to add custom menu items to the top_menu list.
We need to hide this setting entirely, it has caused way too many problems.
Couldn’t we just restrict what can be added to the top_menu to a list of valid items?
Exactly what problems has it given people, I wouldn’t think someone would intentionally place invalid items on this menu, knowing that it would break their site, it says in the description of what to place. I still have this setting on my community, I have no problems with it.
What can go wrong is that a non-existent page can be set as the Discourse homepage. In the example I gave above, Discourse would be trying to find /foobar for the site’s homepage. It wouldn’t be possible to remove foobar from the top menu items, because on refreshing the settings page, ‘foobar’ would not be displayed as a top menu item, but foobar would still be stored internally by Discourse as the first item in the top menu list.
We could and we should, we also should have app safe guards that ensure that this kind of breakage is not possible.
I am fine with nuking this, but we will still need a drop down to select home page, and then people would need a theme component to fuss with top menu.
Easiest thing here is just to remove all the voodoo from that site setting, only allow people to pick from a fixed list and reorganize
WE ALREADY HAVE THIS, it’s called the setup wizard. So frustrated that I have to keep pointing this out over and over…
That is a small percent of what that setting does though, it lets you add “read” “posted” or “bookmarks” and reorganize tabs.
Wizard lets you pick homepage (between latest and categories) it omits “top” as an option which is probably a legit homepage in some cases.
I am adding to my list to kill all the support pain around this which has existed for many years. (and a few other settings that also share pain with this)
Very oddly, we don’t appear to have a proper validator for site settings that ensures a string must match a regex. We have one for string must include regex which is very different. I need this cause the regex will be ^(latest|categories|top) combined with (2), allowing unread and others to be home page causes support issues, they can do that in a plugin.
We don’t appear to have a setting on “list” that ensures you can not add “free form” text to the selections, this is really bad cause post_menu also would use it so you don’t add bogus stuff to post_menu.
The help text is not great
I agree we have suffered here with this setting and post_menu to a degree and want to at least remove this suffering.
I am mostly done here but kind of want to hand this over to @joffreyjaffeux to finish some stuff up here:
Per:
https://github.com/discourse/discourse/commit/88f5251415650f875817c3b3d4a73987a6cd270a
We have an extra allow_any attribute for site setting (default is true). If allow_any is set to false on a list, the choices are restricted so they always match the list.
So for this case, top_menu is now restricted to any combination of:
choices:
- latest
- new
- unread
- top
- categories
- read
- posted
- bookmarks
But… the control used in the UX is now an ugly text box. Since this is a site setting we can live with this, but it is visually somewhat ugly.
That said, you can no longer shoot yourself in foot, which is great.
This commit totally kills 2 features as well:
You can not longer “magic” a category into top menu which we used to support, this has to be done in a theme.
You can not longer use the “magic” - thingy to remove categories from lists, instead you set it in category settings.
Is there a howto for that? I thought I saw one, but my 
skills didn’t suffice to dig it out again – or it never existed and I just hallucinated 
This topic was automatically closed after 4 days. New replies are no longer allowed.