🇨🇦Canada @mparker17

Merged!

I cleaned up some things and added a bit as well. I'll commit shortly. Thanks everyone!

mparker17 → created an issue.

I like @tom konda's approach, i.e.: with the sitemap_metatag submodule providing default configuration when installed!

I think it would be good to add tests though. Tests ensure that future changes to the module by other contributors will not break the functionality that your site depends on.

I think we could add tests by...

1. Adding "drupal/metatag": "^2", to composer.json's "require-dev" section
2. Adding a file modules/sitemap_metatag/tests/src/Functional/SitemapMetatagTest.php, which visits the sitemap and checks that the page text contains the correct values in the canonical_url metatag and title tag. You might find the documentation on Creating Functional tests → helpful.

I am happy to assist with writing tests, if you'd like!

Awesome, thanks @boromino!

(sorry for the late reply, I was on vacation)

Thanks! I'm glad to hear it is working well!

When someone else (i.e.: who didn't write the code) tests the code, and finds it to be working, we mark the issue as "Reviewed & tested by the community", which I'm going to do now!

My customer has a lot of content (~500,000 nodes), a lot of editors, and a lot of jargon (which is often abbreviated); so they have written a web content style guide to help editors write consistently across all those pages — and the guide says when it makes sense to use an abbr tag (or not).

CKEditor Abbreviation helps my customer because it makes it easy to work with abbr tags without editing the page source, which is particularly useful in some of the longer pieces of content (one particularly popular page has 1 abbr tag amongst ~5900 words, at time-of-writing).

Based on the article you linked, I think it makes sense for people using CKEditor Abbreviation to re-evaluate whether abbr still belongs in their style guides — but even if they decide to remove it from their style guide, they will still need a way to work with it in their old content without editing the source code.

I guess what I'm trying to say is, I think it should be up to the end-users of Drupal to decide whether they should be using the abbr tag — but until CKEditor drops support, and/or the W3C removes/deprecates the abbr tag from the HTML specification, I think the CKEditor Abbreviation module will still be useful for people who have abbr tags in their content (even if it's just to help them remove the abbr tags without editing the page source).

@boromino, oops, thank you! All the threads were resolved but I forgot to /approve the merge request. I've done that now. Sorry for the confusion, and thanks for reviewing!

I've pushed a commit with an updated use_case; so I'm moving this back to Needs Review. Reviews welcome! Thanks again!

@jwilson3, thanks for the quick response and help!

I will update the patch to set $use_case = 18 in the new code.

I tried the other other placement options to see if any worked better, and, unfortunately, they did not (details below).

So we will have to stick with _label_help_append_title() for now. (maybe I can submit a patch to autocomplete_deluxe but my initial attempts at a patch haven't been successful).

To be more specific...

_label_help_prepend_field_prefix($element, $content, $use_case);

The label help text wasn't output at all with prepend_field_prefix

_label_help_prepend_description($element, $content, $use_case);

The label help text appeared below/after the input box and above/before the field description, but this isn't ideal, because I need the help text above/before the input box where users will notice it. Put another way, I could get an effect similar to prepend_description by adding a newline to the description itself, so this doesn't really add anything.

_label_help_append_label_suffix($element, $content, $use_case);

The label help text wasn't output at all with append_label_suffix

MR posted; moving to needs review; feedback welcome!

I was able to get something to work by adding the following to label_help_process_form()...

      elseif (isset($item['widget']['target_id']['#type']) 
        && $item['widget']['target_id']['#type'] === 'autocomplete_deluxe'
      ) {
        $use_case = 15;
        $element = &$form[$key]['widget']['target_id'];
        _label_help_append_title($element, $content, $use_case);
      }

I'll post a merge request shortly for review, but...

1. I get the sense from the resulting markup that appending to the title is not ideal, from an accessibility standpoint
2. I'm not certain if I should change the $use_case to a new number

... some feedback from the label_help maintainers would be ideal.

I'm also happy to submit a patch to the autocomplete_deluxe queue if that would make things easier.

mparker17 → created an issue.

Updating the issue summary

We don't have any automated tests for the basicauth, and no specific tests for the standard connector either. Further, search_api_opensearch doesn't have any specific automated tests for its basicauth or standard connectors either.

search_api_solr has test connectors (i.e.: which serve as test doubles in tests), \Drupal\search_api_solr_test\Plugin\SolrConnector\BasicAuthTestSolrCloudConnector, and \Drupal\search_api_solr_test\Plugin\SolrConnector\BasicAuthTestSolrConnector, but the search_api_solr module works in a slightly different way (i.e.: the connectors make the connection to the backend directly; but that's not the case in elasticsearch_connector and search_api_opensearch).

Looking into adding tests for the connectors we have seems non-trivial, so I've created some follow-up issues...

1. ✨ Have ElasticSearchConnectorInterface::getClient() return an Elasticsearch ClientInterface instead of a Client Active
2. ✨ Add tests for the Connectors Postponed

... and I'm going to suggest that we merge this without tests.

mparker17 → created an issue.

Here's a patch for the connector types currently in elasticsearch_connector

Postponing as this is a backwards-compatibility break.

mparker17 → created an issue.

@sunlix, Yes, please add another commit to clean up package.json!

Thank you very much!

For what it's worth, I'm running the updated patch on an 11.2.0 site, and it seems to work fine for me.

Trying to debug the source of the errors led me to a fix!

I've reviewed the changes by @sunlix and I'm satisfied with them. I also contributed to the patch (with my Maintainer hat on), and I'm satisfied with my own changes — but another maintainer (@boromino, @sickness29) should review as well.

Moving to RTBC so another maintainer can review!

My multiple version testing results...

In the table below:

• ✅ means that I was able to see the CKEditor on the node/edit page, I was able to enter an abbreviation using the CKEditor Abbreviation button, and the abbreviation worked on the node/view page
• ❌ means CKEditor did not load on the node/edit page, and thus I was not able to proceed further with testing. I've listed the error shown in the console (but not the stack trace, in part because my JS was minified)

All four sites were set up identically: Standard install profile; CKEditor Abbreviation added to Basic HTML text format; went to /node/add/article to test.

... I'll interpret this in another comment; just wanting to save it here.

Also experiencing a BC break in CKEditor Abbreviation: 🐛 Incompatible with Drupal Core 10.5 / CKEditor 45 Active

@sunlix, thanks I've updated the issue summary.

Maintainer here! Sorry for the delay; it's been a busy few weeks for me.

I have been able to reproduce the problem on Drupal 10.5.0, and can confirm that the code in the patch fixes the issue; but I cannot merge code with broken tests. I also notice some issues in the code: I'll leave some comments in the merge request shortly. For these two reasons I am marking this issue as "Needs Work".

I'll do some more research into compatibility with forward and backward compatibility with Drupal core. There does appear to be some activity in 📌 Update CKEditor 5 to 45.0.0 Active (core), and 🐛 D11.2: Uncaught CKEditorError: Cannot read properties of undefined (reading 'viewUid') Active (editor_advanced_link, another module that integrates with CKEditor) to keep an eye on.

While I was updating the issue summary, I tried to find a link to @ckeditor5/ckeditor5-icons in the NPM Package List, but couldn't. @sunlix, do you have a link to this new package?

Turns out I had made an error in #3522107 which made me think i needed this feature; now that I've fixed the error, I don't think this will be necessary anymore. Thanks for your patience with me!

(Update issue summary, hopefully to get issue reference to work)

(update issue summary now that ✨ Allow a Server plugin to say it doesn't support adding, removing indexes Active exists)

mparker17 → created an issue.

Okay, I have created a merge request with a simple solution. Feedback welcome!

(fix typo in new code snippet)

(Updated the code snippet in the issue summary to use search_api.task_manager to load the tasks by properties)

mparker17 → created an issue.

Ah, interesting, I don't think I've used the Menu Multilingual module → before... I'll try it out on a test site and see if I can reproduce a problem. Thank you!

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 to Active or Needs work.

(thanks for your understanding and patience with me as I try to manage this module's issue queue)

@divya.lakshman,

Apologies for the delay. I created a patch from @berramou in #3085218-20: Filter Menu Tree By Language → but it didn't work for me: after installing the right module ( Menu Manipulator → ), it crashed my site: see my comment in that ticket. Apparently, the same maintainer (@matthieuscarset) also created Menu Manipulator Sitemap → , but that module is independent from this module.

While working on that, I did find a work-around that might work for you depending on your site...

1. Go to /admin/config/regional/content-language. At the top of the page, check Custom menu link to make them translatable. This will cause a Custom menu link details-element to appear further down the page. You can leave most configuration in that Custom menu link details element at their default values, but I unchecked "Changed" because it didn't seem like that needed to be translatable for my tests. I clicked Save configuration on that page.
2. Go to /admin/structure/menu, edit a menu, and edit a menu link. Click the Translate tab at the top of the page. Click Add next to the language(s) that you want to translate that menu link into. Enter a translation and save.
3. Add translations for a handful of menu items for testing. You will want to repeat for all menu items eventually.
4. Go to /sitemap and use the language switcher. You should see the translated text for the menu items that you entered translations for change based on the current language.

This work-around might be sufficient for your use-case if:

1. All of your menu links are custom menu links (i.e.: they were added manually, i.e.: they are not added by a module).
2. You want exactly the same menu items in exactly the same order for all translations.

To make it a bit easier for me to manage this module's issue queue, I'm going to change this issue's status to Postponed (maintainer needs more info), but you can change it back...

If the patch in #3085218-20: Filter Menu Tree By Language → works for you, please let me know by posting that here, and change this issue's status to Fixed

If 3085218 seems unrelated, then please post that here, change this issue's status back to Active, and I will try to help further (I'll probably ask for steps to reproduce!)

If you figure out a solution to your problem (i.e.: Menu Manipulator Sitemap → works for you, or my Content-translation work-around works for you), please post that here, and change this issue's status to Fixed.

If I don't hear back from you in ~6 months or so, then I will close this issue as Closed (cannot reproduce) to make it easier for me to manage the other issues in this issue queue.

Thanks for your understanding!

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)

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.

1. Installed ddev-1.24.6
2. git clone --branch '8.x-2.x' https://git.drupalcode.org/project/sitemap.git && cd sitemap — cloned this module
3. git 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 branch
4. ddev 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 isolation
5. ddev add-on get ddev/ddev-drupal-contrib — installed the very-handy DDEV Drupal Contrib add-on
6. ddev start
7. Edited composer.json, added "drupal/menu_manipulator": "^4" (later I tried ^3) to the require-dev section for testing purposes
8. ddev poser — run composer-install as per the ddev-drupal-contrib setup instructions
9. ddev symlink-project — link the module into place as per the ddev-drupal-contrib instructions
10. ddev 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.
11. ddev drush -y en sitemap — enable the sitemap module
12. ddev drush -y uli — get a login link; pasted in browser

Then I set up the site...

1. I went to /en/admin/config/search/sitemap and enabled the Menu: Main navigation and left its configuration at the defaults.
2. 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.
3. 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.
4. 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).
5. 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.
6. 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 clicked Save configuration on that page.
7. 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.
8. 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.
9. I could now add menu items that were only in Spanish, and could see them on both the English and Spanish sitemaps.
10. I then went to /en/admin/modules and enabled the Menu Manipulator module.
11. 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 the InvalidArgumentException: Class "menu.language_tree_manipulator" does not exist error mentioned at the start of this comment.
12. 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?

Hi Divya, thanks for reporting this!

The issue that you described sounds a bit like the issue reported in ✨ Filter Menu Tree By Language Needs work

The patch in 3085218 needs to be updated before i can recommend that you try it. If you feel comfortable updating the patch yourself, that would be great! But if you would prefer, i can update the patch later today and let you know when it is ready to try.

Update the issue summary to document changes made in this ticket.

Update the issue summary to document changes made in this ticket.

Update the issue summary to document changes made in this ticket.

Update the issue summary to document changes made in this ticket.

Update the issue summary to document changes made in this ticket.

Update remaining tasks.

Update issue summary to note release version, explain API changes.

(fix typo)

Updating issue summary to make it easier for people to see what changed.

(Update issue summary to mention release version)

(Update issue summary to mention release version)

(typo, sorry!)

(update remaining tasks in issue summary to include release)

(Fixed typo in issue summary)

(copy-paste error)

Note this was released in 8.0.0-alpha3 →

Updated issue summary to mention when it was released as well.

Accidentally said it was released in 8.0.0-alpha4 when it was actually released in 8.0.0-alpha3

Updated remaining tasks to show when this issue was released.

Updated remaining tasks to show when this issue was released.

Updated remaining tasks

Updated steps to complete so I can say when it is released.

Updated issue summary to mention when it was published.

Updated issue summary to mention when it was released.

@marckwee, thank you for the contribution! This sounds like a great idea to me!

I committed your code to a merge request to make it easier for me to review. I took a very quick look at your patch: the changes seem clear to me, but I haven't yet had a chance to run the changed code... I may have more feedback when I do.

We should add a change record to assist people upgrading from 8.x-7.x

I didn't see any automated tests in your patch... we prefer to accept merge requests that have passing automated tests. Automated tests ultimately benefit you, because they ensure that future changes to this module (i.e.: by other people) will not break the functionality that your site depends on!

mparker17 → made their first commit to this issue’s fork.

@marckwee, thank you for the contribution!

I committed your code to a merge request to make it easier for me to review.

I took a very quick look at your patch: the changes seem clear to me, but I haven't yet had a chance to run the changed code... I may have more feedback when I do.

I didn't see any automated tests in your patch... the Elasticsearch Connector module maintainers prefer to accept merge requests that have passing automated tests. Automated tests ultimately benefit you, because they ensure that future changes to this module (i.e.: by other people) will not break the functionality that your site depends on!

I don't think that we have any automated tests for Facets yet ( I wrote a manual test → but haven't had the time to turn it into code yet).

Are you interested in learning how to write an automated test?

If you describe how to run a test manually, then I may be able to suggest how to automate it (i.e.: if you're interested in learning to write a test), or write a test myself (if you aren't interested in learning to write a test).

mparker17 → made their first commit to this issue’s fork.

I think that, since we've split the remaining tasks into 📌 Change sitemap.settings.plugins.*.weight from type integer to weight Postponed and 📌 Start using #config_target in SitemapSettingsForm Active , we can mark this as fixed.

I split off...

3. If we dropped support for D9 and D10.0 through 10.1 (i.e.: support 10.2+), we could simplify the configuration form by making use of #config_target in our main config form.

... into its own issue, 📌 Start using #config_target in SitemapSettingsForm Active ; updating the issue summary to say this.

mparker17 → created an issue.

mparker17 → changed the visibility of the branch 3465601-config-schema-2 to hidden.

I split off...

2. If we dropped support for D9 and D10.0 through 10.2, (i.e.: support only D10.3+ and D11.0+ only, we could change sitemap.settings.plugins.*.weight from type integer to weight

... into its own issue, 📌 Change sitemap.settings.plugins.*.weight from type integer to weight Postponed ; updating issue summary to say this.

mparker17 → created an issue.

Updating the issue summary: we are unblocked now, so moving to Needs Work.

Updated issue summary to note we committed the patch and released it in 8.x-2.0-rc1 → .

I merged ✨ Add an option for menu depth Needs review and 📌 Use attributes instead of annotations Active today, so I rebased this branch on top of the latest 8.x-2.x.

Updating Issue Summary's Remaining tasks.

Marking as fixed!

I made a few small tweaks, but otherwise, everything looks good; and manual and automated testing show everything is working great, so I'm going to merge this! Thanks everyone!

Marked as Fixed.

Updated the issue summary with more information, for users of the module who are interested in the change when they find it in the release notes.

Speaking of PHP versions and attributes, I think it's best to explicitly add a PHP requirement (>=8.1) in composer.json.

Makes sense: I've done that in the latest commit.

I expected the Drupal.org Composer Service (façade) a.k.a. project_composer → to update composer.json with a PHP version to match what's in sitemap.info.yml, but to my surprise, I couldn't find any code to do that, so I updated both composer.json and sitemap.info.yml.

If tests pass, then I'm going to merge this. Thanks!

Is this still an issue?

(thanks for your patience with me: I am reviewing old issues to try to keep this module's issue queue in a manageable state)

I think this would be a helpful feature! But I'm not sure how I would implement this, I don't foresee my clients needing this anytime soon, and I'm not sure I have the volunteer time right now, so contributions would definitely be welcome!

I think I'd prefer to have a separate issue for the translatable taxonomy term names, instead of an omnibus issue (especially since one of the items in this issue is blocked by a core issue), so I have created ✨ [PP-1] Support translatable taxonomy term names once TermStorage::loadTree is language-aware Postponed and credited @akalata, @bgilhome, @flocondetoile, and @miiimooo for their work on this issue. I'm going to move this issue back to its previous state. Thanks for your understanding and patience with me!

Crediting @miiimooo for their work on 2613794

Crediting @flocondetoile for their work on 2613794

Crediting @bgilhome for their work on 2613794

Crediting @akalata for their work on 2613794

mparker17 → created an issue.

@julio.raimondi, thank you for filing this feature request!

I think that optional support for the Translatable menu link uri module → would be a great addition to the Sitemap module, but I have no experience working with it, my clients aren't planning to use it anytime soon, and I'm not sure I have the volunteer time right now, so contributions would definitely be welcome!

(aside: removing the non-standard tags as per the issue tag guidelines → )

Note (mostly to myself): I found ✨ Display menu hierarchy as org chart? Active requesting something that sounds similar. I've requested that the person who filed that issue (@damienmckenna) comment here if they think there is some overlap between what they requested and what was built here.

@damienmckenna, recently, @malcomio filed a new ticket with a similar goal — ✨ Provide option to display as a visual sitemap Active — and provided some code.

Would #3525996 suit your use-case? If so, would you be willing to provide feedback on that ticket?

This issue needs my review, so I'm going to move this to RTBC to remind myself to do that. Sorry for the delay; thanks for your patience!

This issue has been Postponed (maintainer needs more info) for 10 months and no new info has been posted, so I'm going to move this issue to Closed (cannot reproduce) for now.

That being said, if you have more information, feel free to re-open the issue!

Thanks for your understanding and patience with me!

@berramou, thank you for the excellent contributions! The code is clear, it makes good use of APIs, and doesn't miss anything as far as I can tell. Manual testing shows everything working as far as I can tell. I don't think this needs any new tests, because the existing tests cover the changes (and they all pass!).

I've rebased this onto 8.x-2.x.

Quick question before I merge this: I noticed that in some annotations in core (for example, in the handlers sub-array of the ConfigEntityType annotation on \Drupal\block\Entity\Block), references to other classes in annotations are done with the special ::class constant... should we also do that for Sitemap's deriver annotation-keys in the following places?

1. modules/sitemap_book/src/Plugin/Sitemap/Book.php,
2. src/Plugin/Sitemap/Menu.php,
3. src/Plugin/Sitemap/Vocabulary.php, and
4. tests/modules/sitemap_custom_plugin_test/src/Plugin/Sitemap/DerivativeSitemapPlugin.php

... for example, in src/Plugin/Sitemap/Menu.php, should we modify the code as follows...

 use Drupal\sitemap\Attribute\Sitemap;
+use Drupal\sitemap\Plugin\Derivative\MenuSitemapDeriver;
 use Drupal\sitemap\SitemapBase;
 /*...*/
 #[Sitemap(
   /*...*/
-  deriver: "Drupal\sitemap\Plugin\Derivative\MenuSitemapDeriver",
+  deriver: MenuSitemapDeriver::class,
   menu: "",
 )]

mparker17 → made their first commit to this issue’s fork.