- π¨π¦Canada mparker17 UTC-4
This sounds more like a feature request than a task to me.
It would be good to know if this is still an issue on Drupal 10, running the latest sitemap version.
(I'm cleaning up old issues as part of a big review of open issues for π± [Plan] Stable 8.x-2.0 release Active : thank you for your patience with me)
- π«π·France berramou
Hello,
yes this is still an issue on Drupal 10.2.X with the 8.x-2.0-beta6 version.
I attach new patch that fix this issue. - Status changed to Needs review
5 months ago 12:07pm 6 August 2024 - π¨π¦Canada mparker17 UTC-4
Moving to "Needs review" so I remember to review the new patch
- Status changed to Needs work
5 months ago 6:26pm 6 August 2024 - π¨π¦Canada mparker17 UTC-4
The patch in #14 introduces an implicit dependency on the menu_manipulator module β , which isn't ideal, so I can't merge patch as-is.
That being said, I see a way to write this patch so that menu_manipulator is an optional dependency...
diff --git a/src/Plugin/Sitemap/Menu.php b/src/Plugin/Sitemap/Menu.php index 312b13d..5dff063 100644 --- a/src/Plugin/Sitemap/Menu.php +++ b/src/Plugin/Sitemap/Menu.php @@ -102,6 +102,14 @@ class Menu extends SitemapBase { ['callable' => 'menu.default_tree_manipulators:checkAccess'], ['callable' => 'menu.default_tree_manipulators:generateIndexAndSort'], ]; + + // If the menu_manipulator module is installed, use its filterLanguage + // manipulator to ensure that only menu items from the current language are + // displayed. + if ($this->moduleHandler->moduleExists('menu_manipulator')) { + $manipulators[] = ['callable' => 'menu.language_tree_manipulator:filterLanguage']; + } + $tree = $this->menuLinkTree->transform($tree, $manipulators); // Add an alter hook so that other modules can manipulate the
... but β as mentioned before β I don't want to merge anything that doesn't include tests, otherwise we won't know if future changes to the sitemap module will break your website!
To write tests for this feature, start by adding menu_manipulator as a test dependency: this ensures that it will be available in the test environment...
diff --git a/sitemap.info.yml b/sitemap.info.yml index 7fb4bb0..a2c7150 100644 --- a/sitemap.info.yml +++ b/sitemap.info.yml @@ -3,3 +3,5 @@ type: module description: 'Display a sitemap.' core_version_requirement: ^9.1 || ^10 || ^11 configure: sitemap.settings +test_dependencies: + - menu_manipulator:menu_manipulator (>=3.0.6)
Then add a Functional test for the functionality. It will look something like the following...
<?php namespace Drupal\Tests\sitemap\Functional; use Drupal\Tests\language\Traits\LanguageTestTrait; /** * Test menu_manipulator only shows items from the current language on sitemap. * * @group sitemap */ class SitemapMultilingualMenuTest extends SitemapBrowserTestBase { use LanguageTestTrait; /** * {@inheritdoc} */ protected static $modules = [ 'content_translation', 'menu_manipulator', 'menu_ui', 'node', 'sitemap', ]; /** * Test menu_manipulator only shows items from the current language on sitemap. */ public function testMultilingualMenu() { // Set up languages. Note English should already be available. // TODO: Might have to set up language negotiation? self::createLanguageFromLangcode('fr'); // TODO: Make menu_link_content translatable? (not sure if necessary) self::enableBundleTranslation('menu_link_content', 'menu_link_content'); // TODO: Make nodes translatable? (not sure if necessary) Note 'page' type should already be available. self::enableBundleTranslation('node', 'page'); // Log in as an admin user. $this->drupalLogin($this->drupalCreateUser([ 'administer sitemap', 'administer menu', 'administer nodes', 'create page content', ])); // Configure sitemap to show main menu. $this->saveSitemapForm(['plugins[menu:main][enabled]' => TRUE]); $englishLinkText = 'EN' . $this->randomString(); // TODO: Add an English-only menu-item with distinctive link text to the main menu. $frenchLinkText = 'FR' . $this->randomString(); // TODO: Add a French-only menu-item with distinctive link text to the main menu. // Switch to a user with minimal privileges. $this->drupalLogout(); $this->drupalLogin($this->drupalCreateUser([ 'access content', 'access sitemap', ])); // Visit the sitemap in English. Check that the English-only item is // displayed on the page. Check that the French-only item is NOT displayed // on the page. $this->drupalGet('/en/sitemap'); $this->assertSession()->linkExists($englishLinkText); $this->assertSession()->linkNotExists($frenchLinkText); // Visit the sitemap in French. Check that the English-only item is NOT // displayed on the page. Check that the French-only item is displayed on // the page. $this->drupalGet('/fr/sitemap'); $this->assertSession()->linkNotExists($englishLinkText); $this->assertSession()->linkExists($frenchLinkText); } }
Aside, in the interest of being transparent, I considered several paths forward:
- Rejecting this patch (I didn't seriously consider this, because that would not help the people reporting this issue)
- Hoping that π Untranslated menu items are displayed in menus Needs work merges soon and solves the problem (2 years ago, @akalata said it would not, but 2466553 has changed quite a bit in the meantime)
- Make sitemap explicitly depend on the menu_manipulator module (i.e.: by adding menu_manipulator to its dependencies in
sitemap.info.yml
)- This means that everyone who uses sitemap would now have to download and install the menu_manipulator module if they want to use the Sitemap menu plugin... if they tried using the menu plugin without menu_manipulator, they would get a WSOD. Other sitemap module users might chafe at having to install a new module (for example, my client uses it only for taxonomy, and therefore wouldn't want to install another module to support a feature that they don't use)
- Introducing a new module dependency (i.e.: on the menu_manipulator module) would be okay in a major version release (i.e.: sitemap-8.x-2.0 -> sitemap-3.0.0), but not in a minor/patch release (i.e.: sitemap-8.x-2.0 -> sitemap-8.x-2.1). Also, a major version release is a major undertaking for me (a volunteer maintainer).
- Note this is different from adding it to the test_dependencies: test_dependencies are only used in the test environment, and it's okay to change those as-needed
- Split the menu functionality into a sub-module that depends on menu_manipulator
- We've done this for the book module now that it has moved into contrib in D11 in π Drupal 11 compatibility fixes for sitemap Fixed
- There's also some discussion in β¨ Compatibility with Domain Menus Active about doing this to add support for the domain_menus module features
- I think, again, sitemap users might chafe at having to install a module they didn't need before, because their site is only available in one language
- Find a way to write this patch so that menu_manipulator is an optional dependency
I ultimately settled on the last option.