Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Bojhan’s picture

Title: Make admin/config/media category » Configuration page: Media category

We have spend a lot of time looking at this specific item, and I think this category can contain a lot of modules that are related to the way Drupal handles files on a site, whatever this file could be - it is merely always considered a Media element on the website. Therefor the category of Media emerged, with the current activities and books around this subject in Drupal subsequently also labeled Media - I think exposes the strength of this label.

We thought about a couple modules, that could fit under this category as well :
*Upload
*Image Assist
*ImageAPI
*FileField
*IMCE

To read the backstory on this change see #546956: [meta-issue] Overhaul of Information Architecture

Bojhan’s picture

FileSize
11.76 KB

Initial patch, probally missing some stuff - as it doesn't show ^^.

I moved file-system.

Bojhan’s picture

FileSize
13.03 KB

Including the suggested change by Catch of creating a new category.

Bojhan’s picture

Status: Active » Needs review
Issue tags: +Usability
FileSize
28.74 KB

Mediacategory.png

Bojhan’s picture

Spoken to a lot of contrib maintainers about the possibility of this category, most seemed fine with their module beign categorized as such.

webchick’s picture

Let's go ahead and move image styles under here as well, as that's another thing in core that clearly falls under this heading, where just "file system" by itself is a poor fit for indicating what this section is for.

Also, let's get some of these contrib module authors actually chiming in on this issue so we can get it marked RTBC.

stella’s picture

I'd put the lightbox2 module in under this category. It's not exactly media, but can't think of a more accurate one.

Bojhan’s picture

FileSize
32.79 KB
28.24 KB

Media_category.png

As webchick suggested, I moved everything else to.

Status: Needs review » Needs work

The last submitted patch failed testing.

Bojhan’s picture

Status: Needs work » Needs review
FileSize
33.83 KB

Fixing tests.

Status: Needs review » Needs work

The last submitted patch failed testing.

Bojhan’s picture

Status: Needs work » Needs review

Tests are failing because of #514172: Increase Max Router Parts

sun’s picture

Status: Needs review » Needs work
+++ modules/system/system.module	16 Aug 2009 21:28:46 -0000
@@ -695,34 +695,43 @@ function system_menu() {
-
-  // Settings.
-  $items['admin/settings/site-information'] = array(
...
+  
+  // Settings.
+  $items['admin/settings/site-information'] = array(

Trailing white-space (blank lines should be blank). Also the reason why those lines are in the patch at all.

Beer-o-mania starts in 14 days! Don't drink and patch.

sun’s picture

No.

+++ modules/image/image.module	16 Aug 2009 22:16:14 -0000
@@ -69,53 +69,53 @@ function image_menu() {
-  $items['admin/settings/image-styles/edit/%image_style'] = array(
+  $items['admin/config/media/image-styles/edit/%image_style'] = array(
...
-    'page arguments' => array('image_style_form', 4),
+    'page arguments' => array('image_style_form', 5),
...
-  $items['admin/settings/image-styles/delete/%image_style'] = array(
+  $items['admin/config/media/image-styles/delete/%image_style'] = array(
...
-    'page arguments' => array('image_style_delete_form', 4, TRUE),
+    'page arguments' => array('image_style_delete_form', 5, TRUE),
...
-  $items['admin/settings/image-styles/edit/%image_style/effects/%image_effect'] = array(
+  $items['admin/config/media/image-styles/edit/%image_style/effects/%image_effect'] = array(
...
-    'page arguments' => array('image_effect_form', 4, 6),
+    'page arguments' => array('image_effect_form', 5, 7),
...
-  $items['admin/settings/image-styles/edit/%image_style/effects/%image_effect/delete'] = array(
+  $items['admin/config/media/image-styles/edit/%image_style/effects/%image_effect/delete'] = array(
...
-    'page arguments' => array('image_effect_delete_form', 4, 6),
+    'page arguments' => array('image_effect_delete_form', 5, 7),
...
-  $items['admin/settings/image-styles/edit/%image_style/add/%image_effect_definition'] = array(
+  $items['admin/config/media/image-styles/edit/%image_style/add/%image_effect_definition'] = array(
...
-    'page arguments' => array('image_effect_form', 4, 6),
+    'page arguments' => array('image_effect_form', 5, 7),

Count again.

This review is powered by Dreditor.

sun’s picture

I'm mistaken, and I should learn to count. :P

quicksketch’s picture

I think we should consider a different way of making sections rather than over-leveraging the menu system for this task. After all, where would "admin/config/media" go to? Is it worth adding a whole item in the path just for adding this section? I think that the URL "admin/config/image-toolkit" would still be completely acceptable, and we should just find a better way of making our sections.

Gábor Hojtsy’s picture

@quicksketch: historically we used to have paths and menu structure reflect the same structure, just like breadcrumbs. You are suggesting we make these inconsistent when compared? So admin/config/image-toolkit would be under admin/config/media in a menu? It is not just display in categories, it is about breadcrumbs, how other menu displayers like the admin_menu and other tools will operate with the menu.

quicksketch’s picture

So admin/config/image-toolkit would be under admin/config/media in a menu?

I'm saying that there's no point in having an "admin/config/media" page at all, since it's just a listing of menu items underneath it (so far just 3 items). The worth of such a page does not merit having an additional menu item at all is what I'm saying. Basically I'm proposing that we come up with another way of grouping items into "Media", "Developer", etc. underneath "admin/config", since having a page that is nothing but a few items in a list is not a page worth having. So there would be no entry in the breadcrumb, menu path, or a page at all for the "Media" category, it would just be displayed in a group on the "admin/config" page.

sun’s picture

I was under quicksketch's assumption, too. But. It's not only about admin_menu as Gábor mentions. The proposed IA has to work without a Toolbar or Administration menu, too. Which makes me very concerned. Did anyone ever tried to use D7 without any of those modules, i.e. the plain menu block? That additional config layer makes me feel like I'd click endlessly to get somewhere.

Gábor Hojtsy’s picture

@quicksketch: how would that grouping be reflected in the breadcrumbs, the management menu block, the admin_menu module's UI, etc? What if the user installs 10 more media modules? Video encoding, podcasts, audio resampling, whatnot? Drupal 7 used to have the International and Development top level items just as listing pages even though International only had 2 items in core and Development only had 1(!).

@sun: you should still be able to use the overview pages just like before; ie: click the top level Configuation and Modules menu item and see *all* categories with all subitems. Same as in D6 with /admin, but this is a different path.

yoroy’s picture

since having a page that is nothing but a few items in a list is not a page worth having.

Fully agree. We have these currently: admin/build, admin/content, admin/settings etc. They are quite useless as a seperate page, it'd be sad to create more of these instead of getting rid of them.

catch’s picture

I also hate those pages (admin/structure is currently one too - turn collapsed mode on and that's probably the most boring page in core right now), however if we don't use the menu structure for this, what do we do? Add a 'category' key to hook_menu()? Add back hook_settings() to register which categories settings pages appear in? None of the alternatives are very appealing.

However, if we did do one of those instead of the system_admin_menu_block(), we'd be able to do a couple of things - 1. trype admin/config/the-settings-path without that extra level of depth 2. Move paths between categories without breaking simpletests at a later date. Both of these would be handy other than losing the intermediate pages.

I'm not convinced about that losing the extra breadcrumb is a problem - if we have a categorized configuration page, with links to individual settings page, it's fine for the breadcrumb to go config > mypage - that's how I got there. Not to mention seven currently doesn't have breadcrumbs, so I can't get back to that intermediate level at the moment anyway.

The only question is whether the individual pages are of any use, as someone who never uses them except by accident I'd say no, and they'll be even less useful when we have 10 categories with 5 items each, rather than the items with 5-10 categories and one with 50 like there usually is on D6 sites.

Gábor Hojtsy’s picture

Its not the immediate pages which are good (and therefore the category pages are not even linked on the admin screen), but the structure they provide in the default menus, the admin_menu, etc. If we are about to introduce another way to categorize pages on top of the menu system just for the admin area, that will be quite confusing.

yoroy’s picture

Well, if we can avoid linking to these pages directly as much as possible, that would be fine with me. Not sure if I can follow catch completely in #22, but if we could even ommit those from the breadcrumbs, that'd be great.

sun’s picture

- The new IA seems to foresee more than settings pages below admin/config. So this proposal would turn into the IA of "a dumping ground for all modules that don't fit anywhere else". You get a list of modules. (And any IA like that would be horrible.)

- Just because Seven doesn't support proper breadcrumbs, this doesn't mean that the better_seven theme will.

Gábor Hojtsy’s picture

@sun: how is that related to the Media category for Media related module settings?

Gábor Hojtsy’s picture

The proposed paths look deep indeed, and they fail on #514172: Increase Max Router Parts. Because for some reason, the discussion of the path was moving on there, I've added my feedback there. I'd suggest to just follow the pattern used by content types and menus, and shorten the path inside the image code: http://drupal.org/node/514172#comment-1934154

dmitrig01’s picture

@Gabor - that's a different aspect of paths - the sheer length. This is talking more about organization of the system.

Gábor Hojtsy’s picture

Ok, so this one with #514172: Increase Max Router Parts seem to both argue the need for and against a "media" component in the path. Because this effectively holds up all IA changes ATM under the "Configuration and modules" section, we need to resolve this disagreement as soon as possible. We've already had a similar discussion on the initial issue on #508458: Config and modules page with David Rothstein, and that ended up using paths instead of other solutions, but we can reiterate and add the new arguments to hopefully get this fixed soon.

*Reasons* for using paths like /admin/config/media, /admin/config/development, etc:

  1. It is in line with Drupal's "navigable URLs", so you get the same URL structure like you navigated
  2. For themes supporting breadcrumbs (yea, Seven is not one of them, see #548806: No breadcrumbs in Seven), the navigation structure shows again
  3. Coding the structure into the menu, all other menu display modules, like admin_menu and the regular management menu display the items categorized as they should
  4. You have a common developer experience, so adding items under the category involves adding an item under one path, just like with any other item in the menu

*Objections* against including the categories in the path like media and development included:

  1. It gets us pointless listing pages which only list a few items; are boring, etc.
  2. Makes paths longer / too long - runs us over our built-in limits for path length, see #514172: Increase Max Router Parts

Ideas to not include these in the path came up in two variants:

  1. Explicitly set plid in the hook_menu definitions so items will not show up in the tree where they were defined. See http://drupal.org/node/508458#comment-1873946 Because plid's are not predictable beyond plid 0, this would only let us move items to the top and not to get over the category definition issue (interestingly it gets us rid of 'config' in the path, not 'media'); if we can figure out the plid dynamically, then we could parent items under a different URL which would solve the long path concern via getting us rid of 'media' but would still need to define the pointless pages which are argued against - or we would not have the items to parent with
  2. Add special handling for 'categories' which is only ever used on the 'Configuration and modules' page, taking a cue from how the left/right block positioning is defined also with special cased menu item definitions. That would require us to have a menu_admin_config_categories kind of database table which defines the categories and some kind of other hook to define these categories outside of the menu, so we get rid of the said pointless pages. Also, we'd need to rewrite all menu handling which deals with paths under the 'Configuration and modules' page to consider these categories when displaying the menu items (ie. use these groupings in the Management menu block and on the administration overview page). All contributed modules would need to special case items under this path similarly.

So far none of them items proposed to get rid of this internal path item but still keep the categorization functioning seemed like a good developer experience, so we kept on using the good old path based hierarchy just like everywhere else to implement these categories.

Now we bumped into the limits of the menu system due to how rich the path usage under the image handling code is implemented and we are at crossroads in continuing with our progress. We need to make a decision to either implement one of these bad hacks or bump the path limit or come up with some other non-hackery to get these categories running in all our menu handler codes and get on with categorization of items.

catch’s picture

IMO the 'media' in the path is more useful/semantic than the 'config' (if we were to move back to paths like admin/development admin/media but otherwise keep things as proposed) - but as in the original issue, either messing with plid = 0 or trying to build a non-path-based category system just to get around this are both worse than the status quo. The current method isn't ideal either with the intermediate pages, but it's what we have.

aaron’s picture

I think that bumping up the path limit sounds like the easiest way to get past this hurdle. As Drupal and its contributed modules become more complex, that will inevitably continue to pop up. I've run into the issue before, which actually forced me to rethink the original architecture of my intended use (for the better), and I'm sure others will as well.

Then we can add the Media category (again, a really great idea I believe, which will easily and quickly balloon from the original 3 in core once a few media handling modules are installed), and get on with it. Any other problems with categorization certainly lie outside of the scope of this issue.

aaron’s picture

To address the actual intention of this issue, I'll note a case where the lack of a good category for this has already caused problems for myself: Embedded Media Field has configuration settings for media, and I made the choice (with no clear guidelines otherwise) to put them under Content rather than Settings. This made sense in my mind, as it's a field for content types. However, with fields in core and likely in the future for users, comments, and who knows what else, this choice becomes more tenuous. I could put it in the already bloated 'Settings' category instead, which has become a catch-all for everything under the sun, but having a Media category would be the most intuitive choice for any module with Media in the name...

Bojhan’s picture

FileSize
33.83 KB

Lets see, Mr. Bot.

Bojhan’s picture

Status: Needs work » Needs review
FileSize
30.72 KB

Mr.Bot, decided to help you a bit.

Status: Needs review » Needs work

The last submitted patch failed testing.

webchick’s picture

Status: Needs work » Needs review

Oh, testing bot...

catch’s picture

Status: Needs review » Reviewed & tested by the community

Looks good.

yoroy’s picture

One of the more obvious categories we can make good use of. Yes please.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Alright, let's go with this then. It looks fine with me. Committed.

tobiasb’s picture

Status: Fixed » Needs review
FileSize
806 bytes

O, where is my filesystem setting page :D

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Verified that this is a good fix.

Bojhan’s picture

Issue tags: +Needs documentation

Dries: Lets not forget documentation.

dhalgren’s picture

ahem...
admin/config/media/file-system
take to
admin/config/development/performance

(the breadcrumb is correct, but the page showed is not)

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD.

tobiasb’s picture

Status: Fixed » Needs review
FileSize
2.51 KB

Followup for image.module :

Updated title args.

remove title callback and title args in ../delete/.. paths, because this do confirm_form().
...drupal_set_title($question, PASS_THROUGH);...

And add "Edit "(thumbnail) /Configure (Scale and crop effect)" to the title arguments, because this sounds better as only "thumbnail" or "Scale and crop effect"

sun’s picture

Status: Needs review » Needs work
+++ modules/image/image.module	22 Aug 2009 21:18:07 -0000
@@ -72,7 +72,7 @@
   $items['admin/config/media/image-styles/edit/%image_style'] = array(
     'title' => 'Edit style',
     'title callback' => 'image_style_title',
-    'title arguments' => array('!name', 4),
+    'title arguments' => array('Edit !name', 5),
@@ -92,7 +90,7 @@
   $items['admin/config/media/image-styles/edit/%image_style/effects/%image_effect'] = array(
     'title' => 'Edit image effect',
     'title callback' => 'image_effect_title',
-    'title arguments' => array('!label effect', 6),
+    'title arguments' => array('Configure !label effect', 7),

Let's keep it consistent:

"Edit [type] !name"

i.e.

"Edit style !name" and
"Edit effect !label"

+++ modules/image/image.module	22 Aug 2009 21:18:07 -0000
@@ -81,8 +81,6 @@
   $items['admin/config/media/image-styles/delete/%image_style'] = array(
     'title' => 'Delete style',
-    'title callback' => 'image_style_title',
-    'title arguments' => array('Delete !name', 4),
@@ -101,8 +99,6 @@
   $items['admin/config/media/image-styles/edit/%image_style/effects/%image_effect/delete'] = array(
     'title' => 'Delete image effect',
-    'title callback' => 'image_effect_title',
-    'title arguments' => array('Delete !label', 6),

Why are those removed?

This review is powered by Dreditor.

tobiasb’s picture

FileSize
4.1 KB
tobiasb’s picture

Status: Needs work » Needs review
sun’s picture

Status: Needs review » Needs work

Returning the result of drupal_set_title() is wrong.

Perhaps, the 'title callback' just have to go.... otherwise, the page callbacks have to set the title, because you can't pass along the second argument to drupal_set_title() through the menu system.

See http://api.drupal.org/api/function/_menu_item_localize/7

tobiasb’s picture

Status: Needs work » Needs review
FileSize
5.38 KB

add nice breadcrumbs, so that the peoples know where they are :D

tobiasb’s picture

FileSize
4.86 KB

ok remove breadcrumbs again, sun means this is an another issue.

sun’s picture

+++ modules/image/image.admin.inc	23 Aug 2009 00:16:40 -0000
@@ -309,6 +295,14 @@
+  if (arg(6) != 'add') {
+    $title = t('Edit %label effect', array('%label' => $effect['label']));
+  }
+  else{
+    $title = t('Add %label effect', array('%label' => $effect['label']));
+  }

Testing for arg() is the outdated style. Just test for !empty($effect['label']) or the internal id instead - new effects won't have that data. :)

I'm on crack. Are you, too?

tobiasb’s picture

FileSize
4.89 KB
sun’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
5.06 KB

Talked back to pwolanin in IRC and this is indeed the only way to fix this issue.

Removed superfluous isset() in combination with !empty().

sun’s picture

webchick asked for clarification, to potentially clarify this in cvs annotate; so here we go:

#550228 by tobiasb, sun: Fixed title callbacks of menu router items of Image
module cannot use %-style placeholders to delimit user-created object title
strings from the generic menu title due to menu system restrictions in
_menu_item_localize(), which always runs check_plain() on custom title
callbacks of menu router items and does not allow to pass the second argument
to drupal_set_title().

webchick’s picture

Status: Reviewed & tested by the community » Needs work

Um. That still doesn't help me. :)

What I'm after is a plain-english summary of your and pwolanin's discussion that would help explain why we're doing drupal_set_title() in a form instead of a title callback.

And then probably we should shorten it to one line and put it above each of those drupal_set_title() chunks because that's a WTF we don't do anywhere else.

sun’s picture

Status: Needs work » Needs review

I've tried to explain that this is nothing special to Image module. See http://api.drupal.org/api/function/node_page_edit/7 for another example of a workaround for this menu system limitation. Due to the mixture of t() placeholders used in the title (other than !foo, i.e. unescaped placeholders), there is no other way than to set the page title in the page callback.

That's a common workaround used in many places throughout core - and - not invented here.

webchick’s picture

Status: Needs review » Needs work

Ok. sun has created #556910: Menu router items: Allow to pass PASS_THROUGH to t() to deal with this menu system regression, which was caused by our more secure drupal_set_title() in D7.

To keep us moving on this, I've committed #54 to HEAD as a temporary fix.

I'm pretty sure these path changes still need to be documented, so leaving needs work for that.

catch’s picture

Priority: Critical » Normal
sun’s picture

Status: Needs work » Fixed

Status: Fixed » Closed (fixed)
Issue tags: -Usability, -Needs documentation, -IA, -D7UX

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