feat: Add LOCALIZE_MENU_ITEMS setting for i18n locale-aware menus (fixes #242)#528
feat: Add LOCALIZE_MENU_ITEMS setting for i18n locale-aware menus (fixes #242)#528fpennica wants to merge 10 commits into
Conversation
jazzband#242) When WAGTAILMENUS_LOCALIZE_MENU_ITEMS = True (or WAGTAIL_I18N_ENABLED = True), menu items automatically resolve to the active-locale page instead of the default-locale page stored in the FK. Two targeted changes to MenuWithMenuItems: 1. AbstractMenuItem.__init__ (menuitems.py) Swaps self.link_page to its localized counterpart immediately after instantiation, so menu_text and the link_page_id cache key are already in the active locale before get_pages_for_display() is called. 2. get_pages_for_display() (menus.py) When LOCALIZE_MENU_ITEMS is active the localized queryset is filtered via an ID-based lookup (p.localized.id for p in base_qs) instead of a direct queryset intersection. The direct '&' intersection would be empty when the active locale differs from the stored locale, causing the menu to render nothing — the root bug confirmed by multiple community reports in jazzband#242 (@Redjam, @fpennica). New setting in conf/defaults.py: LOCALIZE_MENU_ITEMS = False Auto-detection in conf/settings.py: when WAGTAILMENUS_LOCALIZE_MENU_ITEMS is not explicitly set, the value falls through to WAGTAIL_I18N_ENABLED, so projects already using Wagtail's built-in i18n get locale-aware menus with zero extra configuration. All 153 existing tests continue to pass. New test module wagtailmenus/tests/test_localization.py covers: - Setting default value and auto-detection from WAGTAIL_I18N_ENABLED - MenuItem.__init__ swap behaviour (enabled/disabled/same-locale/no-page) - get_pages_for_display() locale-aware queryset building - Regression guard: existing behaviour unchanged when setting is disabled
|
@fpennica , thank you for your PR When you submit a PR, regardless of whether or not a LLM was used, you are responsible for the code you submit. However, once I approve this PR, I will share this responsibility. Up until now, I wasn't aware there was such a challenge with i18n and wagtailmenus. In the past, I simply created different FlatMenus for different locales when I needed to localize menus. I have no idea whether I'm talking to a human right now. Is there one behind this PR? Both the code and the description give me strong LLM vibes. I fact, I have no indication an human ever validated the output of this LLM. So, as it stands, I'm unwilling to accept responsibly for this code. Please open a GirHib Discussion or an Issue. If there really is the opportunity for an i18n improvement here, let's the humans discuss it there first. |
|
Hello @MrCordeiro, I understand your concerns... The issue was discussed in #242 with multiple workarounds proposed. The core idea of this pr was in my comment from 2023: #issuecomment-1462310545 I recently worked on a hobby project with wagtail and applied a similar fix, I just thought it would have been useful to wrap up the fix in a pr. |
Thanks for the response! I'm going to take a look at the discussion and the code in the coming weeks then. I may take a little while, but I'll look into it! One thing I can already see is that the new settings was no included in the docs. Could you add it? |
There was a problem hiding this comment.
Pull request overview
Adds an opt-in (with auto-detection) mechanism to make wagtailmenus resolve menu item pages/URLs against the active Wagtail locale, addressing multilingual setups where menu items currently link to default-locale pages.
Changes:
- Introduces
LOCALIZE_MENU_ITEMS(defaultFalse) and auto-enables it whenWAGTAIL_I18N_ENABLED=Trueunless explicitly overridden. - Attempts to localize
MenuItem.link_pageearly and adjustsMenuWithMenuItems.get_pages_for_display()filtering logic for localized pages. - Adds a new test module covering setting behavior, menu item localization, and regression expectations.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
wagtailmenus/conf/defaults.py |
Adds LOCALIZE_MENU_ITEMS = False default. |
wagtailmenus/conf/settings.py |
Auto-detects LOCALIZE_MENU_ITEMS from WAGTAIL_I18N_ENABLED when not overridden. |
wagtailmenus/models/menuitems.py |
Localizes link_page in AbstractMenuItem.__init__ when enabled. |
wagtailmenus/models/menus.py |
Adjusts get_pages_for_display() filtering to accommodate localized pages. |
wagtailmenus/tests/test_localization.py |
Adds coverage for the new localization feature and settings behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # translations). Bridge the gap by resolving each base page's | ||
| # localized counterpart and collecting their IDs. | ||
| base_qs = self.get_base_page_queryset() | ||
| suitable_ids = [p.localized.id for p in base_qs] | ||
| queryset = queryset.filter(id__in=suitable_ids) |
|
Ok, I'll add docs and test the copilot suggestions in my test project in the coming days 👍 |
|
Well, this got definitely more involved than I hoped... The key change is that menu items are now localized at render time, not at instantiation time. This means the stored FK is never mutated, and we avoid the N+1 query problem when building the prefetch cache. Honestly, this is as much as I can do with my limited skills and knowledge, LLMs or not. I tested this on a couple of sites and it seems to work just fine, but it would probably need more testing. It should be noted that even without directly applying this approach to wagtailmenus, the same localization support should be achieved by simply overriding Feel free to close and discard this if you think it is too much of a hack, I just wanted to share my thoughts and code in case it can be useful to someone else.
|
MrCordeiro
left a comment
There was a problem hiding this comment.
Hi, here are a few extra comments. The PR description is also stale since you dropped the AbstractMenuItem.__init__ approach. Please refresh it.
Would you be able to share a minimal Wagtail project (or even a fixtures + settings snippet) so I can also verify this end-to-end? I'd like to confirm the real-world case - particularly a multi-level menu with a translated parent - against an actual wagtail-localize setup before merging.
Happy to build one myself if you don't have anything reusable lying around, but that would take me some time.
There was a problem hiding this comment.
nitpick: Order menu settings alphabetically.
| from wagtail.models import Page | ||
|
|
||
| from wagtailmenus.conf import settings | ||
| from wagtailmenus.models import MainMenu, MainMenuItem |
There was a problem hiding this comment.
MainMenuItem seems like an unused import
| - ``AbstractMenuItem.__init__`` swaps ``link_page`` to its active-locale | ||
| counterpart so that ``menu_text`` / ``href`` are locale-aware. | ||
| - ``get_pages_for_display()`` builds the prefetch queryset from the | ||
| localized page tree, so multi-level submenus are populated correctly. | ||
| - The final page filter correctly bridges the locale gap (the N+1 query | ||
| path based on ``p.localized.id``) instead of a direct queryset | ||
| intersection that would be empty when locales differ. | ||
| - All existing behaviour is preserved when the setting is disabled. |
There was a problem hiding this comment.
This seem overly verbose and out-of-date ( you are no longer updating AbstractMenuItem.__init__). It's never a good idea to have these comments that explain what the code does - they get outdated fast.
Please reduce the docstring to the minimum a human reader needs to know.
There was a problem hiding this comment.
I feel the the PR's center claim is untested: to sort out @perepicornell 's issue of subpages being prevented from being displayed (see? I've done my homework 😝) of subpages being displayed.
We need a test that:
- creates/fakes a parent page with children,
- maps the parent and its "localized" counterpart with a different subtree,
- asserts that descendants of the localized parent (not the original) appear in
pages_for_display,
| # --------------------------------------------------------------------------- | ||
| # Helper: a descriptor that replaces Page.localized during a test | ||
| # --------------------------------------------------------------------------- |
There was a problem hiding this comment.
Comments like # ---- Helper: ... are noise. Class names + docstrings already structure the file. Please remove them.
|
|
||
| def test_pages_for_display_unchanged_when_disabled(self): | ||
| menu = MainMenu.objects.get(pk=1) | ||
| # The fixture has 12 pages for this menu |
There was a problem hiding this comment.
Same test as test_pages_for_display_count_unchanged
| @override_settings(WAGTAILMENUS_LOCALIZE_MENU_ITEMS=True) | ||
| def test_pages_for_display_unchanged_when_already_in_active_locale(self): | ||
| """ | ||
| When Page.localized returns the same page (already in active locale) | ||
| the output of get_pages_for_display() must be identical to the | ||
| non-localization case. | ||
| """ | ||
| menu = MainMenu.objects.get(pk=1) | ||
|
|
||
| # Empty mapping → all pages return themselves | ||
| mapping = _LocalizedMapping({}) | ||
| with patch.object(Page, 'localized', mapping): | ||
| pages = menu.get_pages_for_display() | ||
|
|
||
| # Still the same 12 pages | ||
| self.assertEqual(len(pages), 12) |
There was a problem hiding this comment.
suggestion: test_top_level_items_unchanged_when_already_in_active_locale and test_pages_for_display_unchanged_when_already_in_active_locale are both just asserting that an empty _LocalizedMapping is a no-op. That's functionally the same as "feature disabled" and is already covered by the regression class. Either drop both or merge into one focused test that proves "feature on + no translations available = identical behavior to feature off."
| with patch.object(Page, 'localized', mapping): | ||
| pages = menu.get_pages_for_display() |
There was a problem hiding this comment.
It the feature is off (empty _LocalizedMapping) this patch is unreachable. Can be dropped and possibly merged with the existing regression.
| # pages_for_display triggers get_pages_for_display(), which populates | ||
| # self._localize_id_map when LOCALIZE_MENU_ITEMS is active. | ||
| pages = self.pages_for_display | ||
| localize_map = getattr(self, '_localize_id_map', {}) |
There was a problem hiding this comment.
This bit made me a bit anxious: _localize_id_map is built somewhere else so there an implicit contract being glued in line 968. The self.pages_for_display only works because pages_for_display is a @cached_property.
If we get_pages_for_display ever carries new side-effect
I was wondering whether we could to extract this computation to a cached_property so that get_top_level_items also computes the map and everyone just reads self._localize_id_map.
| value = super().__getattr__(name) | ||
| # Auto-detect locale-awareness from Wagtail's own i18n flag when the | ||
| # wagtailmenus-specific setting has not been explicitly overridden. | ||
| if name == 'LOCALIZE_MENU_ITEMS' and not value and not self.is_overridden('LOCALIZE_MENU_ITEMS'): |
There was a problem hiding this comment.
Quick question: what is not value guarding against?
is_overridden already tells us nothing was set explicitly, so value has to be the default. If we ever change the default, this check quietly disables the WAGTAIL_I18N_ENABLED fallback and nobody notices.
|
@MrCordeiro thanks for looking into this, I'll try to sort out everything I can over the next few days |
|
Demo/development project for the The point is to demonstrate the difference between:
Quick startUnzip the demo package in the python3 -m venv .venv
source .venv/bin/activate
pip install -e '.[development]' -U
# add wagtail-localize
pip install wagtail-localize
# Run migrations (creates the SQLite DB in the process).
python i18n_demo/manage.py migrate
# Populate the site with a demo page tree and a MainMenu with items pointing to the EN pages.
python i18n_demo/manage.py seed_i18n_demo
# to access the admin panels
python i18n_demo/manage.py createsuperuser
# run server
python i18n_demo/manage.py runserverLayoutLocalization SettingsThe two relevant settings (both in
Page treeThe seed command creates this tree: |
…mments, simplify docstring
|
Main changes:
|
I'm proposing an approach to fix an issue that affects any Wagtail project using
wagtailmenustogether withwagtail-localize(or any multi-locale setup). The issue is that menu items generated bywagtailmenusalways resolve to the default locale URL, regardless of which locale the user is currently browsing.Problem
When a Wagtail project uses
wagtail-localize(orWAGTAIL_I18N_ENABLED = True), menu items always resolve to the default-locale page, because:MainMenuItemstores a FK to the default-localePage.get_pages_for_display()builds a prefetch queryset rooted at those default-locale page paths._prime_menu_item()resolves URLs and text from that prefetched queryset.Result: on
/it/, menu links point to/en/, item text is English, and any second-level submenu is empty (descendants of the Italian page were never prefetched).How wagtailmenus resolves URLs
wagtailmenusstores menu items asMainMenuItemobjects that link to WagtailPageinstances. When rendering, it callsitem.relative_url(current_site, request)which internally calls:Page.get_url()resolves the URL based on the page's own locale, not the request's active locale. A page created in localeenwill always return/en/pagename/regardless of the current request locale.Why the request locale is ignored
Wagtail's
Page.get_url()only considers:localefieldsiteroot path for that localerequestparameter (used for caching, not for locale switching)It does not look at
Locale.get_active()or the request's language prefix. TheLocaleMiddlewaresets the active locale for template rendering and translation, butPage.get_url()has no awareness of it.Why
{% slugurl %}also failsWagtail's built-in
slugurltemplate tag looks up a page by slug and returns its URL. It always returns the first matching page (usually the default locale version). It has no locale-awareness.The gap: no built-in locale-aware URL resolution
Neither
wagtailmenusnor Wagtail's core tags provide a locale-aware way to resolve a page URL from a slug or menu item. Thepageurltag works correctly when you already have a localized page object ({% pageurl page.localized %}), but there's no tag to look up a page by slug in the active locale.Solution
Two targeted changes guarded by a new setting
WAGTAILMENUS_LOCALIZE_MENU_ITEMS.1.
wagtailmenus/conf/defaults.py— new setting2.
wagtailmenus/conf/settings.py— auto-detection fromWAGTAIL_I18N_ENABLEDThe
WagtailmenusSettingsHelperoverrides__getattr__so that whenWAGTAILMENUS_LOCALIZE_MENU_ITEMSis not explicitly set, it falls through to Wagtail's ownWAGTAIL_I18N_ENABLEDflag. Zero extra configuration for projects already using Wagtail i18n.To opt out explicitly even when
WAGTAIL_I18N_ENABLED = True:3.
wagtailmenus/models/menus.pyLocale-aware translation is applied at render time only:
get_pages_for_display()builds the menu's page queryset using a single DB subquery. A lightweightlocalize_id_mapcaches the mapping from stored (default-locale) page IDs to active-locale page IDs.get_top_level_items()useslocalize_id_mapto resolve each menu item to the correct localized page when building the render cache.link_page_idis never mutated — admin saves cannot accidentally persist a swapped value.Why not just fix the template?
Using
{{ item.link_page.localized.url }}in templates only fixes top-level URLs. It breaks multi-level menus becauseget_pages_for_display()prefetches descendants of the default-locale page, so second-level items never appear. The fix must happen server-side before the prefetch queryset is built.Why not use
get_base_page_queryset() & queryset?When
LOCALIZE_MENU_ITEMSis active,querysetcontains localized (IT) pages whileget_base_page_queryset()returns pages for which menu items are configured (EN). The&intersection is empty.