Maximum number of bundle sub-menus does not account for setting zero

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

@simonbaese

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

Thanks

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

@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.

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"

@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.

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.

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 ✨ Links for each media type under 'Add media' are not sorted by label Active (?)

Thanks in advance!

I have updated the MR to resolve the conflicts.

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

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 🐛 Set min and max values for "Maximum number of bundle sub-menus to display" Active .

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.

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!

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)

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?

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 result 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 fix the issue and passes all tests: back to Needs review

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