Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
This is probably the next most obvious category after development, likely candidates:
image toolkits
file system
imagecache presets
Comment | File | Size | Author |
---|---|---|---|
#54 | drupal.image-menu-title.patch | 5.06 KB | sun |
#53 | drupal_550228.patch | 4.89 KB | tobiasb |
#51 | drupal_550228.patch | 4.86 KB | tobiasb |
#50 | drupal_550228.patch | 5.38 KB | tobiasb |
#47 | drupal_550228.patch | 4.1 KB | tobiasb |
Comments
Comment #1
Bojhan CreditAttribution: Bojhan commentedWe 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
Comment #2
Bojhan CreditAttribution: Bojhan commentedInitial patch, probally missing some stuff - as it doesn't show ^^.
I moved file-system.
Comment #3
Bojhan CreditAttribution: Bojhan commentedIncluding the suggested change by Catch of creating a new category.
Comment #4
Bojhan CreditAttribution: Bojhan commentedComment #5
Bojhan CreditAttribution: Bojhan commentedSpoken to a lot of contrib maintainers about the possibility of this category, most seemed fine with their module beign categorized as such.
Comment #6
webchickLet'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.
Comment #7
stella CreditAttribution: stella commentedI'd put the lightbox2 module in under this category. It's not exactly media, but can't think of a more accurate one.
Comment #8
Bojhan CreditAttribution: Bojhan commentedAs webchick suggested, I moved everything else to.
Comment #10
Bojhan CreditAttribution: Bojhan commentedFixing tests.
Comment #12
Bojhan CreditAttribution: Bojhan commentedTests are failing because of #514172: Increase Max Router Parts
Comment #13
sunTrailing 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.
Comment #14
sunNo.
Count again.
This review is powered by Dreditor.
Comment #15
sunI'm mistaken, and I should learn to count. :P
Comment #16
quicksketchI 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.
Comment #17
Gábor Hojtsy@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.
Comment #18
quicksketchI'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.
Comment #19
sunI 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.
Comment #20
Gábor Hojtsy@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.
Comment #21
yoroy CreditAttribution: yoroy commentedFully 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.
Comment #22
catchI 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.
Comment #23
Gábor HojtsyIts 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.
Comment #24
yoroy CreditAttribution: yoroy commentedWell, 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.
Comment #25
sun- 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.
Comment #26
Gábor Hojtsy@sun: how is that related to the Media category for Media related module settings?
Comment #27
Gábor HojtsyThe 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
Comment #28
dmitrig01 CreditAttribution: dmitrig01 commented@Gabor - that's a different aspect of paths - the sheer length. This is talking more about organization of the system.
Comment #29
Gábor HojtsyOk, 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:
*Objections* against including the categories in the path like media and development included:
Ideas to not include these in the path came up in two variants:
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.
Comment #30
catchIMO 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.
Comment #31
aaron CreditAttribution: aaron commentedI 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.
Comment #32
aaron CreditAttribution: aaron commentedTo 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...
Comment #33
Bojhan CreditAttribution: Bojhan commentedLets see, Mr. Bot.
Comment #34
Bojhan CreditAttribution: Bojhan commentedMr.Bot, decided to help you a bit.
Comment #36
webchickOh, testing bot...
Comment #37
catchLooks good.
Comment #38
yoroy CreditAttribution: yoroy commentedOne of the more obvious categories we can make good use of. Yes please.
Comment #39
Dries CreditAttribution: Dries commentedAlright, let's go with this then. It looks fine with me. Committed.
Comment #40
tobiasbO, where is my filesystem setting page :D
Comment #41
Gábor HojtsyVerified that this is a good fix.
Comment #42
Bojhan CreditAttribution: Bojhan commentedDries: Lets not forget documentation.
Comment #43
dhalgren CreditAttribution: dhalgren commentedahem...
admin/config/media/file-system
take to
admin/config/development/performance
(the breadcrumb is correct, but the page showed is not)
Comment #44
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD.
Comment #45
tobiasbFollowup 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"
Comment #46
sunLet's keep it consistent:
"Edit [type] !name"
i.e.
"Edit style !name" and
"Edit effect !label"
Why are those removed?
This review is powered by Dreditor.
Comment #47
tobiasbComment #48
tobiasbComment #49
sunReturning 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
Comment #50
tobiasbadd nice breadcrumbs, so that the peoples know where they are :D
Comment #51
tobiasbok remove breadcrumbs again, sun means this is an another issue.
Comment #52
sunTesting 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?
Comment #53
tobiasbComment #54
sunTalked back to pwolanin in IRC and this is indeed the only way to fix this issue.
Removed superfluous isset() in combination with !empty().
Comment #55
sunwebchick 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().
Comment #56
webchickUm. 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.
Comment #57
sunI'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.
Comment #58
webchickOk. 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.
Comment #59
catchDowngrading, see #653068: Update documentation is incomplete.
Comment #60
sunAdded docs to http://drupal.org/node/727352