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
- Navigate to
/admin/config/user-interface/admin-toolbar-tools - Set Maximum number of bundle sub-menus to display to "0".
- Save the configuration.
- 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.
| Comment | File | Size | Author |
|---|---|---|---|
| #8 | Afterpatch_notfixedmenu2.png | 52 KB | kanchan bhogade |
| #8 | afterpatch_notfixedmenu1.png | 55.24 KB | kanchan bhogade |
| #8 | Afetpatch_fixedmenu.png | 54.25 KB | kanchan bhogade |
| #7 | After Patch Other Menu's.png | 135.53 KB | sandeep_k |
| #7 | After Patch- Structure.png | 192.55 KB | sandeep_k |
Issue fork admin_toolbar-3407811
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
Comment #3
simonbaeseComment #5
prashant.c@simonbaese
Made change to use
!empty(). Please review.Thanks
Comment #6
simonbaeseResolved all threads. Please test your suggestions before pushing code to the branch. Please review.
Comment #7
sandeep_k commented@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:
Comment #8
kanchan bhogade commentedHi
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"
Comment #9
simonbaese@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.
Comment #10
marcoliverLooks 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.
Comment #11
dydave commentedMerge 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!
Comment #12
dydave commentedComment #13
marcoliverI have updated the MR to resolve the conflicts.
Comment #14
dydave commentedRe #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
0would 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!
Comment #20
dydave commentedThe 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_numberand 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
ifchecks 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!
Comment #21
simonbaeseWhat 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 − 239changes?Please see the original reasoning from an earlier comment:
Comment #22
dydave commentedThose 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:
The large number of lines changed results from a code block indentation.
This is the behavior implemented in the merge request.
There shouldn't be a way to select
Unlimitedthough, 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!