Categories below "Configuration and modules"

sun - September 30, 2009 - 02:08
Project:Drupal
Version:7.x-dev
Component:base system
Category:bug report
Priority:critical
Assigned:Unassigned
Status:needs work
Issue tags:API clean-up, D7 API clean-up, Needs usability review, Release blocker, Usability
Description

I'm sorry. I predicted this fact already, but no one wanted to listen.

I'm trying to port http://drupal.org/project/compact_forms to D7 as part of my personal D7 porting effort.

But.

There is no suitable category.

A suitable category would may be "User interface".

But that does not exist.

My little module has, as of now, 609 users.

It doesn't make sense for a module like that to define a new "category".

The "expected" result is to have 4,000+ Drupal modules to define their own "categories".

Core is not able to pre-define the categories.

We already know that. And we really should have learned from admin/build/modules.

I mentioned exactly that before.

The thinking is 2D.

The reality is 3D.

#1

catch - September 30, 2009 - 02:11

I'd put it in 'appearance'. There's nothing else interesting in there at the moment.

#2

sun - September 30, 2009 - 02:12

Are you kidding me? :P

#3

Bojhan - September 30, 2009 - 12:06

User interface sounds good to me, I agree with your understanding that we did not account for 4000+ modules that need their own categories. We tried to account for 50/60% of them, and the other 40% should be handled by carefully choosing the first generation of categories that will pop up. I know its not perfect, or far from ideal even really - but coming up with an IA without knowing the contents will always be a mess.

#4

catch - September 30, 2009 - 12:19

More seriously, since this potentially applies to any form on the site, I'd consider putting it in 'system'. Although it's javascript/user-interface it's also a plumbing module, not completely removed from trigger which is already in there.

#5

yoroy - September 30, 2009 - 12:33
Priority:critical» normal

I second the new 'user interface' category.

#6

sun - September 30, 2009 - 18:09
Priority:normal» critical

Fine. So we create a new "User interface" category.

In the meantime, http://drupal.org/project/compact_forms lives in "Content authoring". You can expect that many contributed modules will similarly just use "anything".

Or they just create a new, arbitrary category, if they feel brave. Like http://drupal.org/project/admin_menu does.

But it goes on.

http://drupal.org/project/render is neither "User interface", nor "Media", nor "Content authoring", nor any other category. That one would indeed probably fit best below "Appearance", but the IA below "Appearance" is not at all prepared for being extended with many advanced configuration pages by contributed modules.

Hence, again, I'll stuff that module "anywhere".

Just not where the user will expect it.

#7

design_dolphin - October 6, 2009 - 20:33

category 'module administration'?

#8

sun - October 9, 2009 - 12:57
Issue tags:+D7 API clean-up

Tagging absolutely critical clean-ups for D7. Do not touch this tag. Please either help with this or one of the other patches having this tag.

#9

sun - October 15, 2009 - 08:21

ok. What I intended, but due to the limited time-frame did not manage to do here is:

  1. Introduce hook_module_category_info(). By default, only system_module_category_info() exists.
  2. system_module_category_info() defines a bloat of around 20 to 30 categories - each with 'title' and 'description', as in:
    <?php
    function system_module_category_info() {
     
    $category['system'] = array(
       
    'title' => 'System',
       
    'description' => 'blah blah blah',
      );
     
    $category['media'] = array(
       
    'title' => 'Media',
       
    'description' => 'bleh bleh bleh',
      );
     
    $category['administration'] = array(
       
    'title' => 'Administration',
       
    'description' => 'bluh bluh bloh',
      );
      return
    $category;
    }
    ?>
  3. As long as no contrib module extends these categories (and there may be only one that may do on behalf of many others, working as a global depedency), only those catgories exist. But at least, it's 20-30 instead of poor 8.
  4. This information is used in two ways: 1) For the modules page. 2) For the menu system.
  5. Modules page is too easy to waste a word on.
  6. Menu system is the part where it gets tricky: Instead of implementing hook_menu() for those administrative items, modules would implement hook_system_menu() (for above example implementation) and the registered menu items would NOT contain the path prefix "admin/config/$category/".
  7. Instead, system_menu() basically invokes module_implements('system_menu'), and for each module, retrieves the internal (machine-readable) category name defined in the $module.info file, consults the category registry, and registers the returned menu router items on behalf of the originally implementing module - and is thereby able to prefix and inject the proper $category into "admin/config/$category/".
  8. This ensures that there is at least a sane amount of usable default categories.
  9. This ensures that module categorization makes any sense after all. It's horribly confusing that you find all core modules in a "Core" category on the modules page, but not in the management menu.
  10. This ensures that modules do not simply create their own categories, nor put themselves into totally different categories.
  11. This ensures that categories are still pluggable, i.e. extensible by contributed modules (though preferable one central contributed module that takes over this job for all modules that do not suit into any of the default categories).

Either that, or we revert this categorized "configuration & modules" part of the new IA.

Anyone who does not see or understand the problem should stop thinking 2D. Drupal is 3D and the IA has to account for that.

#10

catch - October 15, 2009 - 09:21

I came up with a similar approach when thinking about #508458: Config and modules page but at that time it was so close to freeze I gave up trying to take things in that direction, not sure it ever got written down anywhere but encouraging that you mention a similar thing now. Apparently we used to have a hook_settings() which did something similar.

Having this do both the modules page and admin/config sounds good - potentially it could handle /structure and friends too. It's a massive API change though, so we'll need to get Dries/webchick's nod on a very late exception. Note that 508458 is still open because I wasn't at all happy with with the current way we do things, but no-one replied to that issue for the six weeks after I re-opened it.

#11

Bojhan - October 15, 2009 - 09:29

Sorry, but I believe IA fixes can go in UX polish phase so I see no reason to revert. But instead only work harder on this.

I agree that we should probally ship with more default categories, but accusing us of thinking in 2D to me sounds rather stupid. Because we where fully aware, that this IA would cause problems if we didn't have enough programming efforts behind this.

It seems what you are proposing is rather complex, could you explain some more?

#12

catch - October 15, 2009 - 10:05

So the admin/build/modules page doesn't have any pre-defined categories at all, it's just a free-for-all for contrib modules to put themselves in - you can easily end up with user interface, interface, interface enhancements categories if you manage to install three modules which make those slightly differently named categories.

The admin/config page does have some predefined categories, but only ones which directly apply to core modules.

The idea, is to register categories appropriate for both core and contrib in system module, and use these for both pages - those categories would be intended to cover 90% or more of contrib modules, not only those which fit into the ones core modules do.

Modules would be able to add their own category if they absolutely had to, but it'd be much more discouraged.

To implement this, means that instead of defining hook_menu() for their own admin paths, they'd define them in a hook_system_menu, with a 'category' key. system module would then put those in the proper place in the menu system on their behalf.

That means, for example, if we renamed 'config' to 'settings' in Drupal 8, it'd be a few lines change in system.module and updating tests, but no API change for the vast majority of contrib modules.

#13

webchick - October 27, 2009 - 21:45

I was asked to chime in here.

Rolling back the IA is not happening.
Changing the way that admin categories are defined is not happening in D7, because we're past code freeze now.

Seems the sensible way to solve this is to create a "meta issue" for contrib module authors who have no idea where to put their stuff, and we work together to come up with some sensible place-holder categories in core to help guide them along.

#14

sun - October 28, 2009 - 16:50

I shouldn't have mixed co-beneficial changes to admin/config/modules into this issue, because it seems there's quite some confusion here now. I only mentioned admin/config/modules, because we have the identical, multi-dimensional categorization problem on that page since Drupal 5 - but that wasn't so much of a problem, because you only need to work on that page infrequently.

This issue is about the menu system, resp. the new IA below admin/config.

Let me try to explain the current situation and major problem once again:

  • Drupal core provides certain categories below "Configuration and modules".
  • These categories are only available when the implementing module is installed and enabled.
  • Without the implementing module, a category does not exist.
  • Those categories are meta categories. Which means that a large amount of modules should use an arbitrary, but unique and consistent category.
  • Modules that try to re-use a category provided by a Drupal core module need to ensure that the Drupal core module is enabled. Otherwise, the category does not exist.
  • Since these are still meta categories, there is no reason and there should be no requirement for a module to require a core module just to get a predefined category.
  • A module that needs to use a category not provided by any Drupal core module needs to register its own category.
  • Any other module that may intend to use the same category like that previous module needs to register the category on its own, because these are meta categories and any connection or dependency between those modules cannot be presumed.
  • Each module author will therefore have to register a suitable category. Based on my very own D7 porting statistics thus far, less than 25% of all modules fit into the existing categories of Drupal core.
  • Several modules will register their own category, and a Drupal site can easily have 50-150 modules.
  • The result will be an always visible and unusable categorization below admin/config:
    admin/config/administration => Administration
    admin/config/admin => Administration tools
    admin/config/management => Management
    admin/config/user-interface => User Interface
    admin/config/user_interface => UI tools
    admin/config/interface => Interface elements
    admin/config/ui => User interface widgets
    admin/config/e-commerce => E-Commerce
    admin/config/commerce => Shop
    admin/config/ecommerce => Payment tools
    admin/config/cart => Shop integration
    admin/config/ec => E-Commerce
    ...
  • There is no way to defer the responsibility of using proper categories to contrib. If that would work, then we would not have the same categorization problem on admin/structure/modules. There is simply no way to achieve an agreement among 100+ module maintainers to use a consistent category. (And I know what I'm talking about here, because I tried to achieve it once, and that simple change took 1.5 years - and while that issue is marked as fixed, the change is still not deployed everywhere, because a variety of modules still has that change in the development snapshot only.)
  • These menu paths and categories cannot be altered in any way, because all module logic depends on those paths. (form submissions, links, help, conditionally executed logic based on the current path, etc.)
  • Which ultimately means: Whatever decision any module maintainer in contrib makes about the module's category - you have to live and work with that category - regardless of whether there are 5 other categories that intended to introduce the same meta category already, and regardless of whether each category contains one item only.
  • Every single Drupal site having contributed modules installed therefore has a 100% chance to get a weird, confusing, and unusable IA below admin/config like in the example above.

Why a simple approach does not work out:

  • If system_menu() would simply register additional categories, those would be unconditionally displayed by the menu system.
  • There is no way to conditionally and reliably hide parent menu items in the menu system, because the menu system no longer provides a concept of MENU_DYNAMIC_ITEM nor any other special property, which indicates that a menu item should not be exposed when there are no child items.
  • (Re-)introducing a concept like MENU_DYNAMIC_ITEM is a really major challenge, which I highly doubt of being possible anytime soon. That is, because any possible child items also need to be checked for access to determine whether the parent item can be displayed or not. And due to how the menu system works, this check does not end at the immediate/direct children of the parent item - all possibly available items below the parent item need to be taken into account.
  • Without built-in logic and support in the menu system, any other hack will break all contributed modules that render a menu.
  • The benchmark is not the admin/config page, but instead the "Management" menu block, which should not display any empty categories without any modifications to the building and rendering of the menu block in menu.module or system.module.

The proposed solution:

  • Works around the critical bug and misconception of not accounting for the multi-dimensional categorization problem, which was totally known, but simply ignored when the new IA below admin/config was introduced.
  • Works around the limitation of the menu system of not being able to hide certain menu items without accessible children - by not registering them in the first place.
  • Does not break any modules that are already ported to D7, because the categories are plain additional, and hook_menu() is not removed at all. Module maintainers are simply free to change their hook_menu() implementations into hook_system_menu() to leverage one of the new, optional categories defined by Drupal core.
  • Allows the community to create a dedicated, contributed module during the D7 release cycle that can act as single dependency for all kind of modules that belong to the 10% or more modules, which do not fit into any of the additional categories defined by core.

Because all of this, I only see two options to resolve this monster #fail:

  1. Implement the solution as explained above.
  2. Remove the categorization from admin/config/* menu paths.

Note that 2) would be the proper solution from a conceptual and API design perspective, because 1) mainly is a workaround "fix" for an introduced change, which just wanted something - without taking the horrible consequences into account at all. Actually, I'd say that the consequences entirely defeat the intended purpose of the categorization. Instead of a better UX, the actual, resulting categorization makes the UX far worse. The menu router item path in menu system is simply the wrong place to implement categorization.
Someone in IRC raised the question why those categories aren't rather working like Taxonomy, i.e. using user-defined and user-alterable tags instead of hard-coding the crap into the menu system. Not sure whether I'd agree with that proposal, but I definitely agree with the fact that we hard-coded something insanely irrelevant into the menu system.

#15

webchick - October 28, 2009 - 17:35

Thanks for the additional details. I'm slowly warming up to this concept (still need to discuss such a radical API change at this point in the cycle with Dries), but I am curious to know how this actually solves the problem, as long as people can still define any menu path they want to under admin/* through hook_menu()?

#16

sun - October 28, 2009 - 19:00

Short answer: It does not solve the problem.

Long version: It would only be a workaround for a very challenging problem that was simply ignored when people asked for a UI gimmick, which lead to an API change that is technically wrong. The proposed workaround in 1) attempts to solve the problem (but not the cause) by making Drupal core take on the lead of providing a rather large list of predefined categories, which may be consumed by contributed modules.

It is a workaround, because it only works for 90% (or less) of all meta categories that may be required by contributed modules. In general, this fact sheds a very negative light on the architectural implementation, because it is something un-core-ish that would have worked for Drupal 3, but not for Drupal 7. In fact, the entire API change of introducing categories into menu router paths now requires us to architecturally go back to an approach that is similar to hook_settings() we had in Drupal 4 -- except that hook_settings() didn't have the problem of meta-categories, which is the ultimate root and cause of the problem.

To summarize the options once again:

  1. We try to work around a major flaw in the current API by trying to provide as many meta categories as possible to decrease (but not eliminate) the possibility of contributed modules that register new meta categories, which are already registered by another module (but differently). This does not solve the real problem, but provides guidance and a solution for the majority of modules.
  2. We remove the hard-coded categorization from menu router paths, and if absolutely required for D7, we re-think the purpose and goal of the categorization, by either applying it to generated menu links (optionally investigating a tag-alike approach that allows the user to reshape insane/weird/duplicate/confusing/unusable categories), or by applying it to the output of the admin/config page only (which seems to be the primary purpose anyway).

#17

Bojhan - October 28, 2009 - 20:32

We knew that the IA was not done, so I guess this constitutes as the change we need to make. Although my response might be short, I would like to empower this idea - in regards to importance. If we are unable to create a few more very sensible defaults for contrib modules to live in, the whole IA could fall down when we get into the real world of 30 modules enabled. Can we please get some more reviews on this issue though, so we are not making something only tha_sun can understand.

#18

David_Rothstein - October 28, 2009 - 23:33

I had to read this issue a few times before I fully understood the magnitude of the problem, so to help summarize, I've attached a screenshot which helps illustrate the key point, which is this:

If system_menu() would simply register additional categories, those would be unconditionally displayed by the menu system.

As can be seen from the screenshot, this is true: Empty admin categories are automatically displayed right now, and look really bad.

If we could get around that issue, then this problem would be easier to solve (would not require major API changes) because we could just define extra admin/config/somecategory, admin/config/somecategory2, etc the normal way in Drupal core - which core would not use but which would be standard categories (documented somewhere) that contrib modules are all supposed to put their stuff in. However, if we did that now, all of those extra categories would show up on the screen in every single Drupal installation.

***

As for possible solutions that require minimal API changes, I am trying to decide if the following would work:

  1. Notice that in the {menu_links} table, we currently have a 'has_children' column, which as far as I can tell, already stores information about whether a category is empty or not?
  2. So can't we just define a new menu constant (or possibly reuse an existing one) that means "I am a summary page, so if I don't have any children, don't display me in the menu tree"?
  3. Then, core could add several empty categories the normal way in system_menu(), which would not be displayed unless contrib modules fill them:
    <?php
    $items
    ['admin/config/user-interface'] = array(
     
    'title' => 'User Interface',
     
    'description' => 'This is here for contrib modules which need it.',
     
    'type' => MENU_SUMMARY_PAGE,
      ...
    );
    ?>
  4. Because it uses information in the {menu_links} table, this also seems like it would work correctly even if the site admin has rearranged stuff via menu.module, whereas I'm not totally sure how that would work with the proposal in #9.

Does this make sense, or am I missing something totally?

AttachmentSizeStatusTest resultOperations
admin-page-empty-categories.png49.56 KBIgnoredNoneNone

#19

webchick - October 28, 2009 - 23:41

David: That sounds like that'll work for the empty case, but doesn't work for the "Menu has children, but currently logged in user can't access them so for all intents and purposes, it's empty." For that matter, I'm not sure if sun's proposal addresses this either.

We had this issue fixed already in HEAD, but had to roll it back for performance issues. See http://drupal.org/node/296693#comment-1904074.

One one hand, that "Menu has children, but currently logged in user can't access them so for all intents and purposes, it's empty." case isn't addressed in D5/D6 either. But on the other, we're dealing with far fewer possible categories in those older versions.

#20

sun - October 29, 2009 - 00:10

@David: Thanks for reviewing and providing the trigger!

@webchick: Awesome! Thanks for pulling the trigger! :)

For that matter, I'm not sure if sun's proposal addresses this either.

Bam. Thanks, at this point we can forget about the entire workaround proposal.

that [...] case isn't addressed in D5/D6 either.

Tiny correction: MENU_DYNAMIC_ITEM existed in D5, which had an entirely different menu system, and therefore, only D6 does not support that kind of dynamic item that is only to be displayed if there's something.

Ultimately, this means:

  1. Do the impossible and fix #296693: Empty admin categories are not hidden, and afterwards, either implement the proposal outlined here, or implement an API change to re-introduce a new 'type' => MENU_DYNAMIC_ITEM, which allows certain menu links derived from menu router items to disappear in case there are no accessible children. (even typing that sounds veeeery unlikely to be feasible soon)
  2. Accept the regression and revert the categorization below admin/config. Afterwards, either accept that it's too late and it should have been taken into account in the first place, or do a major exception that allows us to work on a different/new categorization feature (and optionally menu system integration) for those items.

#21

David_Rothstein - October 29, 2009 - 00:12

Hm, that is true - I'm pretty sure neither proposal addresses that.

On the other hand, I'm guessing it's not that hard to fix in the particular case of the admin/config page, though? It doesn't seem like there can be a performance issue there, because on that page we have to render the whole thing anyway - so we'll know pretty easily if it's empty or not. So it seems like we can at least make it so that admin/config does not show "empty-for-me-only" categories to those users?

Trying to fix it in the general case (anywhere where the menu tree is rendered for that particular user) - that does sound trickier... but also probably less important?

#22

webchick - October 29, 2009 - 00:23

Unfortunately, fixing it for admin/config is only half of the battle. The "Management" menu is the other half.

#23

catch - October 29, 2009 - 01:33

admin/config already only shows categories with viable links in - that's handled in theme_system_admin_menu_block() and has no serious performance impact - and anyway, if it did, it would only happen when actually viewing that page. The problem is checking expensive access callbacks on the entire admin tree on every page view.

The main reason we have this problem now, is because the toolbar was committed without even cursory benchmarks or performance testing, so we have been firefighting for the two or so months since then. #296693: Empty admin categories are not hidden would not be a serious problem in Drupal 6, because the management menu defaults to the 'admin' item being collapsed - and that item does not use system_admin_menu_block_access(). It would be a problem for admin_menu.module, except for the fact that module implements client-side caching per-user, so doesn't take the hit on every page of building up the menu structure. However the combination of it + the toolbar is a nightmare.

I fully agree this is a critical issue which needs to be solved before release. It should not be solved by re-reverting system_admin_menu_block_access(). Since modules are already porting, a new API vs. reverting to an uncategorized IA is probably equal work.

I think it would also be worth just seeing what an uncategorized admin/config would look like - making use of two column layout etc. rather than the very long thin block which /settings was would still make it a friendlier page. I already get confused where blocks like 'People' are now - and the space they take up means I'm often scrolling the admin panels. A more compact, alphabetical listing, making use of the full available space might not be bad at all. That wouldn't really revert the IA, since the categories were never properly specified in the original wireframes or anything, and we're feeling the effects of that lack of detail now. The main purpose of the IA changes was to move /structure and other paths out of the toolbar, which even though I know sun strongly disagrees with it, he's not arguing to revert here.

#24

Frando - October 29, 2009 - 02:30

What about that. We remove the categories below admin/config from the routher path (so below admin/config we immediately have the individual admin pages). We add two new properties for router items, "categories" and "category".

"categories" is an associative array, like that (all code a little pseudo, no codebase at hand right now):

<?php
function system_menu() {
$items['admin/config'] = array(
  ...
 
'categories' => array(
   
'media' => array(
     
'title' => t('Media'),
      
'weight' => -10,
     ),
   
'services' => array(
     
'title' => t('Web services)',
     
'description' => t('Tools related to web services)',
    ),
   
'default' => array(
     
'title' => t('Other'),
    ),
  ),
);
?>

"category" can then be defined for any child item of admin/config, like that:

<?php
  $items
['admin/config/system-file-system'] = array(
   
'title' => 'File system',
    ...
   
'category' => 'media',
  );

 
$items['admin/config/system-rss-publishing'] = array(
   
'title' => 'RSS publishing',
    ...
   
'category' => 'services',
  );
?>

Both of these properties have to be stored with the router item, of course. Now, whenever a router item defines a "categories" property, the menu tree theme function has to react and sort the loaded children in the tree into the appropriate categories (in a block it could just be presented like a normal tree hierarchy, the categories themselves would just not be links).

The list of available categories can easily be extended via hook_menu_alter(). All category properties are easily alterable. Child items that haven't set the "category" property are sorted into the "default" category. Sorting the items into the categories happens at the theme level, so as all actual config pages (that is items in the admin/config categories) are direct children of admin/config, the access problem should be solved, I think. And it's more of an API addition than an API change.

#25

sun - October 29, 2009 - 03:24
  1. We need to accept that there is no way around fixing #296693: Empty admin categories are not hidden for a multitude of reasons, not limited to this issue.

    I also want to amend that admin_menu is not the only "administrative menu rendering module" in contrib. There is also SimpleMenu, DHTML menu, and all the other related modules that are listed on Administration menu's project page. That is why the issue needs to be properly fixed within the menu system and must not be special-cased for an arbitrary function in system.module.

  2. To prevent any misunderstandings in advance: I am, and it seems also the UX team is, highly against this
        'default' => array(
          'title' => t('Other'),
        ),

    or
        'system' => array(
          'title' => t('System'),
        ),

    (where "System" is a "meta" category that holds items like "Shortcuts" - WTF?)

    The entire purpose of introducing categories in the first place was to provide a meaningful guide to the functionality you may search. If the functionality is just stuffed anywhere, then the whole categorization #fails.

    Likewise, this issue is about preventing contributed modules from registering the same meta category multiple times. Hence:

    How ever the parent link container (MENU_DYNAMIC_ITEM) issue is going to be solved, the three-dimensional problem of registering meta categories will remain -- unless all meta categories are basically just tags, which the site administrator can quickly alter via the UI, which is unlikely to happen for D7.

  3. As mentioned before, I'm favor of catch's and frando's direction of removing the categorization from menu router paths, because it technically is the wrong API to implement categorization. If we would want that, then we would have node/category/% instead of node/%, which would be equally wrong, because these are menu router paths.

    (To clarify: Drupal works very well without menu links, but not without menu router paths.)

#26

JoshuaRogers - October 29, 2009 - 04:15

Subscribe

#27

David_Rothstein - October 29, 2009 - 04:25
Status:active» needs work

OK, I am starting to get very confused and am a bit convinced that we are now discussing two or three totally separate bugs all in the same issue... so I decided to just write some code :)

The attached patch implements what I described:

  1. In the patch:
    • Core defines an "admin/config/example" category but does not populate it with any children.
    • Because it has no children, this category does not display either in the Management menu or on the screen at admin/config (see first screenshot).
    • If you use the Menu module to drag something into it so that it is no longer empty, it then automatically appears in both places (second screenshot).
    • Similarly, if you were to install a contrib module that provided "admin/config/example/mymodule", the category would also automatically appear.

    Therefore, this solves the basic problem raised in the issue. All we now need is a list of X number of categories along the lines of "admin/config/example" - which we can define using the standard hook_menu() method.

    The patch also needs some tweaking because there are about 40 different ways that menu trees get rendered in Drupal and I'm not sure I got all of them :) But as the screenshots show, the important ones are taken care of.

  2. This patch does not fix the problem at #296693: Empty admin categories are not hidden, but neither does it make it any worse than it already is. That is a totally independent bug. While theoretically, one could imagine a solution to that problem that also automatically fixes this one, it sounds like we probably can't count on that due to the performance issues.

    Ripping apart the new Drupal 7 IA, however, will do absolutely nothing to mitigate that problem either. Taking the toolbar module out of core (or totally rewriting it) might, however :)

  3. @catch wrote:

    admin/config already only shows categories with viable links in - that's handled in theme_system_admin_menu_block()

    Maybe it did that at one point, but it definitely doesn't do it now -- if it did, I wouldn't have had to put a workaround for it in the attached patch :)

AttachmentSizeStatusTest resultOperations
hide-empty-summary-pages-591682-26.patch6.32 KBIgnoredNoneNone
example-category-empty.png142.74 KBIgnoredNoneNone
example-category-not-empty.png151.56 KBIgnoredNoneNone

#28

sun - October 29, 2009 - 14:49

The problem of that approach is:

<?php
function menu_tree_page_data($menu_name, $max_depth = NULL) {
...
       
// Build an ordered array of links using the query result object.
       
$links = array();
        foreach (
$query->execute() as $item) {
         
$links[] = $item;
        }
...
     
// Check access for the current user to each item in the tree.
     
menu_tree_check_access($data['tree'], $data['node_links']);
}
?>

i.e. a category that is empty, because the current user does not have access to the items in it, is still displayed.

To summarize once again and hopefully make it clear:

  1. #296693: Empty admin categories are not hidden is required to reliably and correctly hide empty category/grouping items. When that has been fixed, we could simply register many more categories in system_menu() that will use the re-introduced menu router item type MENU_DYNAMIC_ITEM, so the menu tree building functions have a target to apply the special behavior to.
  2. If we believe that #296693 can and will be fixed soon, then this issue could be considered as fixed after introducing those further meta categories, which hopefully will solve the underlying problem of multi-dimensional registered categories for perhaps 90% of all modules.
    However, that approach will still be a workaround, because we already know that it won't work for all modules (100%).

    Hence, the decision that has to be made for this point is whether we trust that 1) can and will be fixed soon and we can live with a half-baked architecture -- ignoring that menu router paths are the totally wrong location to implement categorization.

#29

David_Rothstein - October 29, 2009 - 15:14

Yeah, like I said above, I make absolutely no claim that my patch fixes the problem at #296693: Empty admin categories are not hidden -- it does not :)

However, I guess what I still don't understand is why these issues are necessarily coupled: As far as I understand, the bug at #296693: Empty admin categories are not hidden has existed since Drupal 6.0. Why is it suddenly a release blocker now?

If someone is, for example, using admin_menu in D7, hovers over "Configuration & Modules", gets a dropdown menu with 8 or 9 items in it, but 3 of those items are empty because they, personally, do not have access to anything in it - is that really the end of the world? It's definitely not great, but people do understand the concept of empty folders, I think.

Whereas the problem you have identified in this issue is definitely new to Drupal 7 (given the way we are asking modules to conform to a new IA) and seems much, much more serious.

#30

catch - October 29, 2009 - 16:17

David, you're right, seems no-one bothered to test this when rolling #551080: List non-container items on /admin for a complete overview, which I've now re-opened.

#31

pwolanin - October 30, 2009 - 00:26

It's sounds like there are a couple small bugs (with separate issues) to be fixed - but otherwise I'm not sure we should make further changes to the core API

#32

Bojhan - November 4, 2009 - 22:45
Title:Categories below "Configuration and modules" make no sense» Categories below "Configuration and modules" - need more default categories

Apart from changing the menu system and more API related issues, I would like to pull a more generic title - as we try to do what is proposed earlier. If this is better served as a new issue please notify me.

#33

Bojhan - November 8, 2009 - 21:44
Title:Categories below "Configuration and modules" - need more default categories» Categories below "Configuration and modules"

#627080: [meta-issue] Additional categories admin/config For the actual IA work, so the implementation can continue here.

#34

webchick - November 8, 2009 - 22:34

We had a lengthy chat about this in #drupal today.

Sun's objections boil down to the fact that we encode categorical data in the menu path. Menu categorization is highly subjective; while core will go one way in order to try and hit the 80% mark, something like Open Atrium or Buzzr or Gardens may want to go a totally different way. Currently, your only option is to hook_menu_alter() the path from admin/config/development/XXX to admin/config/butterfly/XXX. But when you do that, suddenly all of your help text/drupal_set_message() links are broken, the tests (which are all pointing to the default path) no longer pass, etc. It also means that unless core defines an appropriate top-level menu path, contrib has to do so itself, and you might end up with 30 contributed modules all defining variations of admin/config/user-interface admin/config/interface admin/config/ui... Just like our lovely module "package" key. :P

For these reasons, I can get behind the idea of de-coupling the path itself from its categorization. The main reason we do this at all currently is because our default breadcrumb generation system is limited to only deriving breadcrumbs based on a hierarchical path. But we can do special tricks with drupal_set_breadcrumb() and system_admin_block() (or whatever it is) to help mitigate this.

However, we need to store that categorization somewhere. We can't go back to admin/config having 37,000 links in one gigantic list (esp. now that basically everything goes under config). Of the proposed options, the one that makes the most sense to me is simply adding a 'category' index to hook_menu(). I realize this is really ooky because it conflates page routing data with menu link display data, but it's not like this is anything new; we also have title, description, position, context, theme...

I know Peter is opposed to this, and I take his opinion as menu system maintainer very seriously. But I asked him if we were to split hook_menu() into hook_page_router() [for path, * callback and * access] and hook_menu_link() (for everything else) in Drupal 8 whether or not we could store a category association in hook_menu_link() and I believe his answer was yes (I'll let him speak for himself). It certainly makes sense to me though; the idea would be that hook_menu_link() would contain any presentational logic related to the link display itself, and from that respect fits well with the other items in that list (well, except theme which is just a bit weird :P).

So I caution against us defending too rigidly against a separation here that doesn't actually exist in reality. That said, I'm certainly open to debate on how else to store this category info, but of the presented options, this is my favourite.

#35

David_Rothstein - November 9, 2009 - 03:41

Menu categorization is highly subjective; while core will go one way in order to try and hit the 80% mark, something like Open Atrium or Buzzr or Gardens may want to go a totally different way. Currently, your only option is to hook_menu_alter() the path from admin/config/development/XXX to admin/config/butterfly/XXX

Maybe I'm just not getting something, but:

1. If you want to use a different URL, you can add it as a URL alias.
2. If you want to create an "admin/config/butterfly" category, you can do that in hook_menu().
3. Finally, if you want to take an existing menu item and recategorize it into your new butterfly category, you can do this by dragging and dropping it via the menu.module UI..... (Granted, I have never tried doing the equivalent of that via writing code, but if Drupal lets you do it via drag and drop, surely it lets you write code for it as well :)

So there doesn't seem to be a new issue here that requires massive changes. The only problem we currently have is that if core wants to define a bunch of (empty) categories like admin/config/butterfly for contrib modules to use, currently they will be displayed even when they are empty, and we don't want that. The patch above solves part of that problem, and (at least the last time I checked) the patch at #296693: Empty admin categories are not hidden seemed to solve the rest... both via similar methods which could probably be combined, and overall without any major API overhauls.

Is this a reasonable summary or am I missing something?

#36

sun - November 11, 2009 - 00:07

All of the mentioned "workaround" suggestions in #35 are fully in line with the problem: They all talk about menu links, NOT menu router paths.

To technically re-phrase them correctly:

1. If you want to make the same path available under a different URL, you could add a URL alias. (has nothing to do with this issue)
2. If you want to create an "admin/config/butterfly" category, you can create a new menu link. (so also breadcrumbs will work properly)
3. If you want to take an existing menu item and recategorize it into your new butterfly category, then you can move the link via menu.module's UI.

4. If a module wants to re-categorize itself to a more sane default, then it won't need to rewrite all redirects, help arguments, etc. and it won't break a ton of other contributed modules that depend on the paths by doing so.

#37

yoroy - November 22, 2009 - 22:46

I've tried to group modules 100 to 300 into some categories. Should at least give us some idea on which ones we want to add by default.

http://spreadsheets.google.com/pub?key=tWJHejIEuEGpeRk4a-SI_bQ&single=tr...

 
 

Drupal is a registered trademark of Dries Buytaert.