- π¨π¦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
10 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
10 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.
- π¨π¦Canada mparker17 UTC-4
I've converted the patch in #14 into a merge request, and I made the changes I requested in the first part of #16, but, when I installed menu_manipulator-3.1.0 or menu_manipulator-4.0.0 (I tested both), when I go to
/sitemap
, I get a WSOD, and the error...InvalidArgumentException: Class "menu.language_tree_manipulator" does not exist. in Drupal\Core\DependencyInjection\ClassResolver->getInstanceFromDefinition() (line 32 of core/lib/Drupal/Core/DependencyInjection/ClassResolver.php).
... and I'm not sure where to go from here, because I don't think I understand the Steps to Reproduce!
Here's what I did to test...
I started by setting up a test environment.
- Installed ddev-1.24.6
git clone --branch '8.x-2.x' https://git.drupalcode.org/project/sitemap.git && cd sitemap
β cloned this modulegit remote add sitemap-3085218 https://git.drupalcode.org/issue/sitemap-3085218.git && git fetch sitemap-3085218 && git checkout -b '3085218-filter-menu-tree-by-language' --track sitemap-3085218/'3085218-filter-menu-tree-by-language'
β switched to this issue's branchddev config --project-type=drupal --docroot=web --php-version=8.3 --corepack-enable --project-name=sitemap
β set up a ddev site for testing the module in isolationddev add-on get ddev/ddev-drupal-contrib
β installed the very-handy DDEV Drupal Contrib add-onddev start
- Edited
composer.json
, added"drupal/menu_manipulator": "^4"
(later I tried^3
) to therequire-dev
section for testing purposes ddev poser
β run composer-install as per the ddev-drupal-contrib setup instructionsddev symlink-project
β link the module into place as per the ddev-drupal-contrib instructionsddev drush -y si demo_umami
β install Drupal with the Umami Food Magazine demo content β normally I use the minimal install profile, but I know the Umami demo is a multilingual site, so I figured it would be a good test case for this issue.ddev drush -y en sitemap
β enable the sitemap moduleddev drush -y uli
β get a login link; pasted in browser
Then I set up the site...
- I went to
/en/admin/config/search/sitemap
and enabled theMenu: Main navigation
and left its configuration at the defaults. - I went to
/en/admin/structure/menu/manage/main
. I saw that the Menu language =English
. It struck me that maybe you're intended to use entirely different menus in each language, but the issue summary and discussion in this ticket seemed to imply that there were menu items for more than one language in the same menu, so I went to figure out how to do that. - I checked the Translate menu tab, but I could only translate the title and administrative summary of the menu itself, i.e.: not the menu items.
- Editing the items in the menu, I saw they were "special" (i.e.: managed programmatically), so I disabled them (i.e.: by unchecking them and clicking "Save"). I then re-created custom menu links to the same locations (i.e.:
<front>
,/en/articles
, and/en/recipes
). - While creating/editing my new custom menu links, I did not see a language selector, (nor a Translate primary action), so it was unclear to me how to get menu links from multiple languages in the same menu.
- I know menu links are content entities, so I went to
/en/admin/config/regional/content-language
to check out the settings there. The Custom menu link entity-type was not set to be translatable, so I checked it to make it translatable. I left most configuration in the Custom menu link details element at the default values, but I unchecked "Changed" because it didn't seem like that needed to be translatable. I clickedSave configuration
on that page. - I went back to
/en/admin/structure/menu/manage/main
and edited a custom menu link. Now I could see the language selector. I also saw a Translate tab. - To see what would happen, I used the Translate tab to add a Spanish translation for some of the menu links. Then I refreshed
/en/sitemap
, and/es/sitemap
in a separate tab. To my surprise, I saw the English translations for the menu items on the English sitemap, and the Spanish translations for the menu items on the Spanish sitemap. It strikes me that making Custom Menu Links translatable, and adding translations for them might be a more-sustainable work-around for some sites. That being said, I'm aware of long-standing bug π Untranslated menu items are displayed in menus Needs work , and I have also worked on highly-localized sites where there completely different content in different languages, down to different menu structures and orders, so I'm aware this won't work for everyone. - I could now add menu items that were only in Spanish, and could see them on both the English and Spanish sitemaps.
- I then went to
/en/admin/modules
and enabled theMenu Manipulator
module. - Since I had already the new code in place (i.e.: from step 3), I decided to refresh
/en/sitemap
, and/es/sitemap
to see if it made the untranslated menu links go away. This is when I got theInvalidArgumentException: Class "menu.language_tree_manipulator" does not exist
error mentioned at the start of this comment. - I tried going to
/en/admin/config/user-interface/menu-manipulator
to see if there were any settings to change, but I got a different WSOD,ArgumentCountError: Too few arguments to function Drupal\Core\Form\ConfigFormBase::__construct(), 1 passed in /var/www/html/web/modules/contrib/menu_manipulator/src/Form/MenuManipulatorSettingsForm.php on line 88 and exactly 2 expected in Drupal\Core\Form\ConfigFormBase->__construct() (line 44 of core/lib/Drupal/Core/Form/ConfigFormBase.php).
Now I'm not sure what to do next!
Can someone experiencing this problem help me to figure out how to reproduce the error on a fresh site install?
- π¨π¦Canada mparker17 UTC-4
For people who prefer patches, I've created a patch of the current state of the merge request.
(if you want to contribute code to this issue please contribute in the merge request: I'm only providing these patches as a convenience this time, to help me answer a question in another issue β thanks for your understanding)
- π¨π¦Canada mparker17 UTC-4
Since I have asked for someone experiencing this problem help me to figure out how to reproduce the error on a fresh site install, I'm going to change this issue's status to
Postponed (maintainer needs more info)
. If you can help me, please post it here, and change the Status back toActive
orNeeds work
.(thanks for your understanding and patience with me as I try to manage this module's issue queue)