Problem/Motivation

In /admin_toolbar/admin_toolbar_tools/src/Plugin/Derivative/ExtraLinks.php there is a constant MAX_BUNDLE_NUMBER which defines the maximum number of bundles (or sub-menu's) that will be generated - for example the number of sub-menus under Content Types or Taxonomy.

This is not documented, so it was not clear at first why the sub-menu's did not include all my content / taxonomy types.

Trying to manipulate the menu manually - the menu reacted unpredictably. Menu items manually added would be removed / changed.

Proposed resolution

Increase this value from 10 to 20, make the MAX_BUNDLE_NUMBER a configuration option and at the very least document this.

Remaining tasks

  • Add a configuration form for the module
  • Add a configuration option for Maximum Number of Sub-Menu's
  • Update the documentation: mention this setting
  • Increase the default from 10 to 20
CommentFileSizeAuthor
#54 interdiff_52-54.txt8.42 KBraman.b
#54 3094835-54.patch24.32 KBraman.b
#52 3094835-52.patch17.09 KBromainj
#51 3094835-51.patch16.09 KBromainj
#47 3094835-47.patch17.64 KBromainj
#41 3094835-41.patch18.2 KBromainj
#39 3094835-39.patch12.68 KBromainj
#37 3094835-37.patch12.08 KBromainj
#36 screenshot.png62.54 KBromainj
#23 admin_toolbar-Decrease_toolbar_height--8.x.patch389 bytesDuneBL
#22 3094835-22.patch17.02 KBidebr
#22 interdiff-19-22.txt12.71 KBidebr
#19 configure_max_bundle-3094835-19.patch12.83 KBTomazetti
#17 configure_max_bundle-3094835-16.patch10.33 KBHardik_Patel_12
#15 configure_max_bundle-3094835-15.patch15.09 KBhash6
#13 configure_max_bundle-3094835-12.patch16.68 KBhash6
#12 configure_max_bundle-3094835-11.patch13.45 KBhash6
#3 Configure_max_bundle_number-3094835-3.patch10.77 KBHardik_Patel_12
#6 Configure_max_bundle-3094835-6.patch10.77 KBHardik_Patel_12
#7 Configure_max_bundle-3094835-6.patch11.17 KBHardik_Patel_12
#8 interdiff_7-8.txt8.84 KBhash6
#9 configure_max_bundle-3094835-8.patch12.89 KBhash6
#10 interdiff_7-9.txt8.84 KBhash6
#11 configure_max_bundle-3094835-11.patch13.57 KBhash6
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jpdippenaar created an issue. See original summary.

Hardik_Patel_12’s picture

Assigned: Unassigned » Hardik_Patel_12
Status: Active » Needs work
Hardik_Patel_12’s picture

Kindly apply this patch , it works

Hardik_Patel_12’s picture

Assigned: Hardik_Patel_12 » Unassigned
Status: Needs work » Needs review
Hardik_Patel_12’s picture

Hardik_Patel_12’s picture

New patch for above issue

Hardik_Patel_12’s picture

Final Fixed added service dependency in patch

hash6’s picture

FileSize
8.84 KB

Reviewed the above patch and added few changes pertaining to position of menus and clearing linting errors.Attaching the interdiff for the same.

hash6’s picture

hash6’s picture

FileSize
8.84 KB
hash6’s picture

hash6’s picture

hash6’s picture

hash6’s picture

hash6’s picture

Berdir’s picture

Status: Needs review » Needs work

You are missing default configuration and schema and an update function that sets the default, that's why tests are failing.

Hardik_Patel_12’s picture

Status: Needs work » Needs review
Tomazetti’s picture

Added #16 and the missing form.

andypost’s picture

Makes sense to provide "configure: admin_toolbar_tools.settings" to module's info.yml

+++ b/admin_toolbar_tools/admin_toolbar_tools.routing.yml
@@ -79,6 +79,14 @@ admin_toolbar_tools.theme_rebuild:
+admin_toolbar_tools.settings:
...
+    _form: '\Drupal\admin_toolbar_tools\Form\AdminToolbarToolsSettingsForm'

could be added more visibility on modules page

hash6’s picture

Assigned: Unassigned » hash6
idebr’s picture

Assigned: hash6 » Unassigned
Issue tags: -Change the MAX_BUNDLE_NUMBER from a constant to a configuration option
FileSize
12.71 KB
17.02 KB

Attached patch implements the following changes:

  1. Removed the uninstall hook: config is removed on uninstall automatically.
  2. Moved the menu link to the 'User interface' configuration.
  3. Updated wording of the configuration option.
  4. Updated the automated tests.
  5. #20 Added a configure key to the info file.
DuneBL’s picture

#22 failed on last dev:

patching file admin_toolbar_tools/README.txt
patching file admin_toolbar_tools/admin_toolbar_tools.info.yml
patching file admin_toolbar_tools/admin_toolbar_tools.install
patching file admin_toolbar_tools/admin_toolbar_tools.links.menu.yml
patching file admin_toolbar_tools/admin_toolbar_tools.routing.yml
patching file admin_toolbar_tools/admin_toolbar_tools.services.yml
patching file admin_toolbar_tools/config/install/admin_toolbar_tools.settings.yml
patching file admin_toolbar_tools/config/schema/admin_toolbar_tools.schema.yml
patching file admin_toolbar_tools/src/Form/AdminToolbarToolsSettingsForm.php
patching file admin_toolbar_tools/src/Plugin/Derivative/ExtraLinks.php
Hunk #3 FAILED at 48.
Hunk #4 succeeded at 66 (offset -1 lines).
Hunk #5 succeeded at 76 (offset -1 lines).
Hunk #6 succeeded at 92 (offset -1 lines).
Hunk #7 succeeded at 317 (offset -1 lines).
1 out of 7 hunks FAILED -- saving rejects to file admin_toolbar_tools/src/Plugin/Derivative/ExtraLinks.php.rej
patching file admin_toolbar_tools/src/SearchLinks.php
patching file tests/src/Functional/AdminToolbarToolsSortTest.php
patching file tests/src/FunctionalJavascript/AdminToolbarSearchTest.php

Note also that it could be interresting to change the padding of the menu items because when MAX_BUNDLE_NUMBER is higher then a lot of item are hidded.

see the attached small patch to achieve this.

romainj’s picture

As discussed in different issues like https://www.drupal.org/project/admin_toolbar/issues/2908747 we do not want to have any configuration for the Admin Toolbar module. We wish to keep it plug and play.
As for the documentation I will add some explanation on the module page.

DuneBL’s picture

@romainj ok, what about increasing MAX_BUNDLE_NUMBER and decreasing the padding like in my patch?
For example, If you use bootstrap_paragraph, by default you will not see all the paragraph type which is not very handy

andypost’s picture

@DuneBL then it sounds better to have as theme variable (same config under the hood)

romainj’s picture

As we're having more and more complains about the number of items to display, we're going to make it a state value. So that number will be an option for each instance of a site and not a configuration to deploy.
I will provide a patch later on.

idebr’s picture

#27 Can you elaborate why it should be state, rather than configuration? When using configuration synchronisation, the configuration is identical over different environments, so the preferred amount of items to display will be identical as well.

romainj’s picture

@idebr I think that the number of items to display depends on the size of your screen and it's not really a configuration of the site. On a project each developer would be able to choose how many sub-menus they want to appear. Moreover it is more a visual option than a feature option.

idebr’s picture

#29 Sure it is dependent on the size of your screen, but what if you are looking at the same site? Visual configuration is also configuration, a typical example is Views configuration.

Berdir’s picture

I'm also not sure how state would be better here, if it's really something per user/developer then it would need to be local state/storage in the browser like some other things, but this is something that's decided at the time of route building I think, so you can't have different users with different settings on the same site? At best you could visually limit it further down from the configured setting.

alison’s picture

Am I understanding correctly -- "state" would be like the "show/hide row weights" thingy all over the Drupal UI?

(If yes) For this number-of-bundles-to-show situation, is the idea that it would be a similar toggle, like, "show all"/"show 10" (or whatever the default max number is)?

And, would it be for each entity type, or would it be a site-wide toggle (like row weights)?

I get the principle of the module being plug-and-play, but isn't it sufficiently plug-and-play in that you don't *have* to do anything after turning on the module? (Or is it that exporting/codifying a config entity is "doing something"? If so, I can understand that, it definitely is "doing something," just trying to understand.)

One way or another, I desperately want to be able to see all my ctypes in the admin menu. It drives me crazy I don't have access to as many menu levels in D8 as I did in D7, I'm still miffed about that haha -- so when I could no longer get to all my bundles from the admin menu, I was devastated all over again!

Thanks, all :)

JasonSafro’s picture

Dear Admin Toolbar Team,

Thank you for making a great module. It seems like you put in a lot of work and made something that is very useful. I've been wondering for months why my menu items kept disappearing. I finally sank the time in today to backtrack it to the MAX_BUNDLE_NUMBER constant.

I love that your module is plug-and-play. But, I hate that I can't configure it. It seems like a lot of people feel the same way. At least 20 users took time to backtrack the issue to the constant and comment on here; I bet hundreds more users are experiencing the same problem but don't know what to do about it.

It sounds like you're being very flexible and trying to make some changes; that's great! But, your solution seems pretty complicated. What about a really simple approach:

  • Add a configuration form
  • Use a default value if no configuration is set

This feels like a really simple approach that would make people happy and is easy to implement.

Best,
Jason

webdrips’s picture

Nicely said @JasonSafro; I agree it would be nice to have the option to increase this.

I did increase it in my distribution to 20, and decreased the padding a bit for submenu items, and can now see how 20 will easily fit now (but the menu is still quite readable IMO).

DuneBL’s picture

If there are too many submenu to fit in the window we should be able to scroll down to see the hidded submenu. This is not the case right now.

romainj’s picture

FileSize
62.54 KB

Patch #23 has also display bug.

romainj’s picture

I fixed the patch #22.

Status: Needs review » Needs work

The last submitted patch, 37: 3094835-37.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

romainj’s picture

DuneBL’s picture

Looks like the form is missing.
Here is what I get after applying the #39 patch:

InvalidArgumentException: Class "\Drupal\admin_toolbar_tools\Form\AdminToolbarToolsSettingsForm" does not exist. in Drupal\Core\DependencyInjection\ClassResolver->getInstanceFromDefinition() (line 24 of core/lib/Drupal/Core/DependencyInjection/ClassResolver.php).
Drupal\Core\Controller\HtmlFormController->getFormObject(Object, '\Drupal\admin_toolbar_tools\Form\AdminToolbarToolsSettingsForm') (Line: 76)
Drupal\Core\Controller\FormController->getContentResult(Object, Object)
call_user_func_array(Array, Array) (Line: 123)
romainj’s picture

DuneBL’s picture

Status: Needs work » Reviewed & tested by the community

I confirm #41 apply correctly and is doing the job.
Some idea:
1-When reaching first time the config form, there is no default value... I think we can add one
2-Create a setting to choose a small or a large padding:

.toolbar-tray a.large-padd { padding: 1em 1.3333em;}
.toolbar-tray a.small-padd { padding: 0.5em 1.3333em;}

Depending of the setting a class could be added to the <a> tag
3-Much more complicated: allow to scroll down the menu

The last submitted patch, 39: 3094835-39.patch, failed testing. View results

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 41: 3094835-41.patch, failed testing. View results

romainj’s picture

@DuneBL yes there is a default value but you need to run the admin_toolbar_tools_update_8201() function first (drush updb).

DuneBL’s picture

I have tried to updb but I got the following:
PHP Fatal error: Cannot redeclare admin_toolbar_tools_update_8201() (previously declared in .../modules/contrib/admin_toolbar/admin_toolbar.install:22) in .../modules/contrib/admin_toolbar/admin_toolbar_tools/admin_toolbar_tools.install on line 20

function exists twice

romainj’s picture

DuneBL’s picture

Status: Needs work » Reviewed & tested by the community

#47 apply cleanly and runs has expected; I have tested the db update and the default value is set to 20

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 47: 3094835-47.patch, failed testing. View results

DuneBL’s picture

Another thing I am thinking about: the label Maximum number of bundles per entity type to display is a very technical description; this setting could be changed by a user who don't know anything about bundles. More over, I think this setting slice other menus...
What not using something like Maximum number of menu ? (or sub-menu)

romainj’s picture

FileSize
16.09 KB
romainj’s picture

Now the Admin Toolbar Search module has a dependency to the Admin Toolbar Tools module due to the use of the configuration settings.

DuneBL’s picture

#52 failed on last dev:

patching file README.txt
patching file admin_toolbar.info.yml
Hunk #1 succeeded at 1 with fuzz 1.
patching file admin_toolbar_search/admin_toolbar_search.info.yml
Hunk #1 FAILED at 1.
1 out of 1 hunk FAILED -- saving rejects to file admin_toolbar_search/admin_toolbar_search.info.yml.rej
patching file admin_toolbar_search/admin_toolbar_search.services.yml
patching file admin_toolbar_search/src/SearchLinks.php
patching file admin_toolbar_search/tests/src/FunctionalJavascript/AdminToolbarToolsSearchTest.php
patching file admin_toolbar_tools/admin_toolbar_tools.info.yml
Hunk #1 succeeded at 1 with fuzz 2.
patching file admin_toolbar_tools/admin_toolbar_tools.install
patching file admin_toolbar_tools/admin_toolbar_tools.links.menu.yml
patching file admin_toolbar_tools/admin_toolbar_tools.routing.yml
patching file admin_toolbar_tools/config/install/admin_toolbar_tools.settings.yml
patching file admin_toolbar_tools/config/schema/admin_toolbar_tools.schema.yml
patching file admin_toolbar_tools/src/Form/AdminToolbarToolsSettingsForm.php
patching file admin_toolbar_tools/src/Plugin/Derivative/ExtraLinks.php
raman.b’s picture

Status: Needs work » Needs review
FileSize
24.32 KB
8.42 KB

Resolving the failed test cases from #52

romainj’s picture

Patch #54 works for me. Thanks @raman.b

webdrips’s picture

#54 Worked for me thanks

jpdippenaar’s picture

As the person who originally logged the request - I do apologise for all the effort; I tested this and it is working perfectly for me. Thank you.

  • romainj committed f08ad1f on 8.x-2.x
    Issue #3094835 by romainj, hash6, Hardik_Patel_12, idebr, raman.b,...
romainj’s picture

Commited into the lastest dev version. Thanks to all.

romainj’s picture

Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.