🇨🇦Canada @mparker17

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.

(I promised to move the issue to Needs Work in the previous comment but forgot to select the State)

@malcomio, thank you for the contribution!

Later, I will do a more-in-depth code review, and test the new functionality; and I may have some more feedback at that time. For now, I took a very very brief look at the merge request, and noticed some things...

1. cspell doesn't recognize the word brailsford's - you can fix this by adding the word to .cspell-project-words.txt
   1. For more information, see the CSpell on Drupal.org documentation.
2. stylelint has raised 66 CSS style issues - you can fix many of them automatically by passing --fix to stylelint.
   1. Aside: I use ddev/ddev-drupal-contrib to make stylelint available in my local development environment - with that running, the command ddev stylelint --fix should fix most issues.
3. I didn't see any automated tests in the merge request... the Sitemap maintainers prefer to accept merge requests that have passing automated tests. If you need help writing tests, please ask!
   1. I think you could check that the CSS file is included when the new option is checked; and not-included when the option is unchecked. tests/src/Functional/SitemapCssTest.php does something similar for sitemap.theme.css.
   2. 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 also have some deeper questions...

Item 4 (i.e.: continuing from the ordered list above)

Regarding the use of Matt Brailsford's CSS Sitemap, I don't see a license in that repo. For better or worse, we are only allowed to host code that explicitly has the GPL 2.0+ license on drupal.org → (and, even if we find a project whose license is compatible, when we copy that other project's code into the Sitemap project, it becomes the Sitemap module maintainers' job to monitor the upstream repo and keep Sitemap module's copy up-to-date).

The recommended way around this is to use the Libraries module → , i.e.: declare the third-party code as a library, rely on the site's maintainer to download the library, and use the Libraries module to link to it properly.

Item 5

In the issue description and comment #2, you gave ~5 different visual sitemap options (i.e.: GlooMaps-style, Canva-style, MermaidJS-style, Slickmap-style, and Matt Brailsford's CSS Sitemap), but you didn't really explain why you chose Matt Brailsford's CSS Sitemap to be the default.

For what it's worth, I think Matt Brailsford's CSS Sitemap looks nice — but as the module's maintainer, I have to explain to the other 36,000 sitemap-2.x users → why we picked it over the other 4 options presented here.

One way that we could make the decision to pick a default easier for the other sitemap-2.x users to accept is by making it easier for them to use their own (alternative) visual style. We could do this in several ways:

1. Update the module documentation to explain how to use the libraries-override → directive in their theme to change the visual style.
   1. This is definitely the easiest option, but I daresay we might have to be prepared to answer the question "why didn't we use libraries-override to override the existing sitemap.theme library instead of adding a separate visual style?"
2. Change the configuration key from visual_css to something like visual_css_matt_brailsford, to make it easier for users to see how they could patch in their own visual_css_gloomaps, visual_css_canva, etc.
3. Provide an API for other modules to easily register their own visual css library and select it on the sitemap options page.
   1. This is definitely the most-complex option; but would allow people with other visual sitemap preferences to add their own sitemap_mermaidjs, sitemap_slickmap, etc. modules to the Sitemap module's ecosystem → (and/or easily create a custom module for their own site theme).

Pending answers to these deep questions; fixes for the cspell and stylelint lints; and automated tests, I am going to move this to "Needs work". That being said, please me know if there is anything that I can do to help!

Looks good to me, merging!

@ajetiv1, for now, the changes are in merge request !4, attached to this issue.

If you'd like a patch (e.g.: for Composer), there are instructions to download a patch in the Using GitLab to contribute to Drupal documentation on Downloading a patch file → .

Thanks everyone for your hard work on this issue!

I'm taking a look at the merge request and trying to understand what it does...

I don't think I've run into a Data Source plugin → before, so please forgive me for the silly question (and for using Elasticsearch-specific terms), but does a Data Source plugin tell Search API how to work with an Index with a dynamic mapping and/or an Index whose content is ingested by third-party crawler (e.g.: Elastic web crawler or Apache Nutch)?

I've added some code in !91; it needs tests; but reviews are welcome.

mparker17 → created an issue.

@alfattal, awesome, I have pushed the commit! Reviews welcome!

@alfattal, I've figured out a way to make the visibility of the "Items selected" details box independent from the visibility of the "Select / Deselect all results (all pages)" checkbox.

I'm uploading it as a patch because it goes a bit further out-of-scope than what @jerrylow originally proposed as their own solution to the issue they filed (also, Hi @jerrylow! 👋). If the other people in this issue agree that this is the direction that they want to go, I'll simply push the commit from my local.

I've uploaded a patch to 4.3.x as is tradition; as well as an interdiff from the most-recent-commit to the merge request (commit 51e5c5c2) to show the proposed changes for someone already familiar with what's in the merge request.

Feedback (from everyone in this issue, as well as the maintainers) is welcome!

You can see in the attached screenshots that I've replaced the "Force show selection info state" with two controls: a control to change the visibility of the "Items selected" details element, and a separate control to change the visibility of the "Select / Deselect all results (all pages)" checkbox.

Screenshot of config page and resulting view, configured to only show the selection box:

Screenshot of config page and resulting view, configured to only show the select all checkbox:

Screenshot of config page and resulting view, configured to show neither selection control

Screenshot of config page and resulting view, configured to show both selection controls:

@alfattal, got it, thanks! I'll take a look!

Yes but from what I remember there may be more conditions. Should check first.

@graber, I apologize, but are you asking me to check if there are more conditions? (I'm happy to try, but I'm asking for clarification because I don't want to be a bottleneck)

Moving back to "Needs Review"

I've pushed an update hook to fix existing views (and I wrote a test for the update hook).

I also put a Choice constraint on the force_selection_info configuration

Looking at the configuration form control, I find it a bit confusing.

    $form['force_selection_info'] = [
      '#type' => 'select',
      '#title' => $this->t('Force show selection info state'),
      '#default_value' => $this->options['force_selection_info'],
      '#description' => $this->t('Should the selection info fieldset be shown above the view even if there is only one page of results and full selection can be seen in the view itself?'),
      '#options' => [
        '' => $this->t('Default Behavior'),
        'show' => $this->t('Always show'),
        'hide' => $this->t('Always hide'),
      ],
    ];

At the very least, I would recommend that we change the key/label of the "Default" option — neither the key (i.e.: an empty string) nor the label ("Default Behaviour") describes what it does very well. Maybe something like 'only_multiple' => $this->t('Only when there are multiple pages'),?

I daresay the title and description could be reworded as well, but I don't want to derail the discussion either.

@graber what are your thoughts as a maintainer?

To prove to myself what was going on, I added a test.

With the patch in this issue...

1. The 'Default Behavior' (i.e.: force_selection_info = '') is to:
   1. hide the 'Select / deselect all results (all pages, N total)' checkbox (i.e.: input name="select_all") when the table is empty (0 results)
   2. hide the 'Select / deselect all results (all pages, N total)' checkbox (i.e.: input name="select_all") when the table has 1 page of results
   3. show the 'Select / deselect all results (all pages, N total)' checkbox (i.e.: input name="select_all") when the table has 2+ pages of results
2. The 'Always show' (i.e.: force_selection_info = 'show') is to:
   1. hide the 'Select / deselect all results (all pages, N total)' checkbox (i.e.: input name="select_all") when the table is empty (0 results)
   2. show the 'Select / deselect all results (all pages, N total)' checkbox (i.e.: input name="select_all") when the table has 1 page of results
   3. show the 'Select / deselect all results (all pages, N total)' checkbox (i.e.: input name="select_all") when the table has 2+ pages of results
3. The 'Always hide' (i.e.: force_selection_info = 'hide') is to:
   1. hide the 'Select / deselect all results (all pages, N total)' checkbox (i.e.: input name="select_all") when the table is empty (0 results)
   2. hide the 'Select / deselect all results (all pages, N total)' checkbox (i.e.: input name="select_all") when the table has 1 page of results
   3. hide the 'Select / deselect all results (all pages, N total)' checkbox (i.e.: input name="select_all") when the table has 2+ pages of results

Given the above behavior, regarding the comment in #27...

The patch in #25 works fine. However, it doesn't provide the option to disable the (Select / deselect all results) which is what this ticket is basically about.

My understanding is that the 'Always hide' (i.e.: force_selection_info = 'hide') is a way to disable the "Select / deselect all results" checkbox? But maybe @alfattal is talking about something else, and I am misunderstanding #27?

We should also add tests.

Interesting, I don't see changes in any patches or merge requests that address the issue identified in #27 📌 Provide a way to disable "select all on all pages" Needs work .

I've turned the patch by @knyshuk.vova in #25 📌 Provide a way to disable "select all on all pages" Needs work into a merge request.

Next, I'll address the issue identified in #27 📌 Provide a way to disable "select all on all pages" Needs work .

mparker17 → changed the visibility of the branch 4.3.x to hidden.

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

Merging!

Yay, the new test passes!

Updating the issue summary with some more-detailed information to make it easier for people reading the release notes when this actually gets released.

@tom konda, I rebased the branch onto 8.x-2.x. Then I added an update hook (sitemap_update_8205()) that adds a menu_depth to all existing menu plugins. I also updated SitemapUpdateSettingsTest::testUpdateHookN() to test the update hook. Testing out the functionality manually, it seems to work the way I expect, which is excellent. I still need to review the code properly; but overall this is looking good. Thank you!

I've added a tests/src/Functional/DashboardListBuilderTest.php in the latest commits to !59. Reviews welcome!

Wait, it seems like there were some other errors. Re-opening

This looks good to me; merging.

Going to mark ✨ Deprecate the constructors in BlocksController, TaxonomiesController, and MenuLinksController Active as the parent of this issue, because we have to do that one first :)

(moving back to "needs review" to get feedback)

@plopesc wrote,

Removing the "View" local task was made on purpose because having the ability to create or edit a dashboard does not give the access to it.
That's why the "Preview" local task was created, ensuring that users that can create/edit the dashboard can get access to preview, even if they don't have access to it from /admin/dashboard local tasks.

Regarding the penyaskito's approach, that could be an option, but that link would point to a 404 error if the current user does not have access to the dashboard. We might just only make links to those dashboards the current user has access to, but it could be confusing.

Interesting... I hadn't considered that specific scenario.

@penyaskito's buildForm() code alters the table row to add the link... would it be an acceptable compromise to check $entity->toUrl('canonical')->access() before transforming the label into a link, like this...?

public function buildForm(array $form, FormStateInterface $form_state) {
    $form = parent::buildForm($form, $form_state);
    foreach ($this->entities as $entity) {
      $dashboardUrl = $entity->toUrl('canonical');
      if ($dashboardUrl->access()) {
        $form[$this->entitiesKey][$entity->id()]['label'] = [
          '#type' => 'link',
          '#title' => $entity->label(),
          '#url' => $dashboardUrl,
        ];
      }
    }
    return $form;
  }

mparker17 → created an issue.

@penyaskito, @plopesc, thank you for the quick replies; I apologize for my own delay in getting back to you!

@penyaskito wrote,

🐛 Wrong redirect after Dashboard layout edition Active would definitely help here. Once you save, you are redirected to the dashboard canonical url instead of the listing.

I agree, this would help, as it would have cleared up some of my own misunderstanding while I was testing.

@penyaskito wrote,

The toolbar caching is definitely an issue, can you create a separate one?

Okay! I will edit this message and post a link here when I've done so.

@penyaskito wrote,

An option could be having on DashboardListBuilder:

  public function buildForm(array $form, FormStateInterface $form_state) {
    $form = parent::buildForm($form, $form_state);
    foreach ($this->entities as $entity) {
      $form[$this->entitiesKey][$entity->id()]['label'] = [
        '#type' => 'link',
        '#title' => $entity->label(),
        '#url' => $entity->toUrl('canonical'),
      ];
    }
    return $form;
  }

I like this code better; I'll change my merge request shortly!

I'll reply to @plopesc's message soon.

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