Problem/Motivation

It is not possible to set zero ("0") in the Maximum number of bundle sub-menus to display setting in the Admin Toolbar Tools configuration. When zero is set, all bundle sub-menu items are displayed. The expected behavior would be that no bundle sub-menu items are shown.

Steps to reproduce

  1. Navigate to /admin/config/user-interface/admin-toolbar-tools
  2. Set Maximum number of bundle sub-menus to display to "0".
  3. Save the configuration.
  4. All bundle sub-menu items are shown.

Proposed resolution

Currently, ->pager() and array_slice are used to limit the respective menu items. These handle the zero-case differently. Therefore, we should introduce separate checks for the zero-case.

Remaining tasks

Review code changes.
Communicate change of behavior in release notes.

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

simonbaese created an issue. See original summary.

simonbaese’s picture

Status: Active » Needs review

Prashant.c made their first commit to this issue’s fork.

prashant.c’s picture

@simonbaese

Made change to use !empty() . Please review.

Thanks

simonbaese’s picture

Resolved all threads. Please test your suggestions before pushing code to the branch. Please review.

sandeep_k’s picture

StatusFileSize
new276.18 KB
new192.55 KB
new135.53 KB

@simonbaese, I've tested the MR- MR !65 mergeable on the Drupal - 10.2 version, the patch was applied successfully. After applying the patch, the issue was fixed partially for me, The issue was fixed for the "Structure menu's" list but not applied on other menus- sharing attached before & after results for reference-

Testing Steps:

  • Enable/Install Admin Toolbar Module.
  • Go to> Admin>Configuration/User interface/Admin Toolbar Tools settings-
  • Set the value of 'Maximum number of bundle sub-menus to display' to 0 & save.
  • Check the Menus for the applied changes- Added Before Results.
  • Download the Patch & Apply.
  • Check the Menus after applying the Patch- Added After Results.
kanchan bhogade’s picture

Status: Needs review » Needs work
StatusFileSize
new54.25 KB
new55.24 KB
new52 KB

Hi
Tested MR !65 on the drupal 10.2
The patch applied successfully...

Test Result:
The issue fixed partially using the MR !65
After the patch fix is applied for Structure some menus and other menus are not fixed.

Attaching screenshot for reference
#7 comments
Moving to "needs work"

simonbaese’s picture

Status: Needs work » Needs review

@Sandeep_k & @Kanchan Bhogade Please read the issue description carefully. This issue aims to fix a bug in the current implementation. It does not intend to extend or change the menu behaviour. The max bundle number setting is currently not used for user bundles. Please open another issue, if you like to change that and do not bloat this issue. Please describe the expected behaviour when posting screenshots. Especially the second set of screenshots it is not clear.

marcoliver’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me! The issue fork solves the problem described in the issue body.

Comments #7 and #8 may be valid, but are out of scope for this task.

Marking RTBC.

dydave’s picture

Status: Reviewed & tested by the community » Needs work

Merge request shows conflicts.

Could somebody please re-roll the merge request for the latest version on 3.x?

Additionally, this looks somewhat related to #3444550: Links for each media type under 'Add media' are not sorted by label (?)

Thanks in advance!

dydave’s picture

marcoliver’s picture

Status: Needs work » Needs review

I have updated the MR to resolve the conflicts.

dydave’s picture

Category: Bug report » Feature request
Related issues: +#3408119: Set min and max values for "Maximum number of bundle sub-menus to display"

Re #11: Sorry, I meant, this issue seems to somewhat be conflicting with #3408119: Set min and max values for "Maximum number of bundle sub-menus to display".

I understand what we're trying to do here: Allow users to disable the display of bundle links.

Which isn't possible with the module currently and seems like a valid request.

It could be argued that 0 would be used to represent unlimited, but in this case how could none be displayed?
If users really needed an equivalent to unlimited they could just enter a very big number, or a max of 500, as suggested in #3408119: Set min and max values for "Maximum number of bundle sub-menus to display".

So, let's try to get this added to the module 👍

I'll take a bit more time to give this another review/round of tests and report back.
Cheers!

dydave changed the visibility of the branch 3407811-maximum-number-of to hidden.

dydave changed the visibility of the branch 3407811-disable-entity-bundle-links to hidden.

dydave changed the visibility of the branch 3407811-disable-entity-bundle-links to active.

dydave changed the visibility of the branch 3.x to hidden.

dydave’s picture

The previous merge request was not taking into account all the impacts of the implementation of this feature.

Created a new merge request above at #15 with a more complete coverage of the changes required to:

Skip any processing of entity bundle links if 'max_bundle_number' is set to '0'.

Searched module's code base base for max_bundle_number and found several occurrences where the variable was used.
In previous merge request, the checks for the variable value were done a bit late: Moved the if checks a bit up in the code and indented the corresponding code blocks.

This way, a lot of other entity routes also get excluded and further enforce the setting.

All PHPUNIT Tests still seem to be passing 🟢

Any feedback, comments or suggestions would be greatly appreciated.
Thanks in advance!

simonbaese’s picture

Status: Needs review » Needs work

What is going on here? This is not a feature request. How are we going from a merge request that more or less changes two lines to a merge request with + 245 − 239 changes?

Please see the original reasoning from an earlier comment:

  • $max_bundle_number = NULL enter this conditional (the user did not specify a number in configuration, therefore, "unlimited" bundles)
  • $max_bundle_number > 0 enter this conditional (the user specified a maximum, therefore use the pager in query)
  • $max_bundle_number === 0 do not enter the conditional (The user set zero, therefore, no bundles should be loaded)
dydave’s picture

Status: Needs work » Needs review

How are we going from a merge request that more or less changes two lines to a merge request with + 245 − 239 changes?

Those two lines were not enough unfortunately and left a lot more links displayed, in particular in the Admin Toolbar Search and 'All Types' links, which doesn't seem to be consistent if entity bundle links are not displayed in the menu.

Please read above comment #20 carefully:

Skip any processing of entity bundle links if 'max_bundle_number' is set to '0'.

Searched module's code base base for max_bundle_number and found several occurrences where the variable was used.
In previous merge request, the checks for the variable value were done a bit late: Moved the if checks a bit up in the code and indented the corresponding code blocks.

This way, a lot of other entity routes also get excluded and further enforce the setting.

The large number of lines changed results from a code block indentation.

Please see the original reasoning from an earlier comment:

This is the behavior implemented in the merge request.
There shouldn't be a way to select Unlimited though, see above #14 (out of scope in this ticket).

The patch still stands, as it should properly address this issue and passes all tests: back to Needs review.

Any testing feedback, comments and questions would be greatly appreciated.
Thanks in advance!