Fix dashboard as the default /admin local task

Hello. Yes it should.

yay, at last :P

Ok, so can we just get a patch?

A first re-roll, the block tests still fail and I'm unsure why. It seems that the default theme is Stark and so the blocks aren't found.

The other tests failed because they accessed 'admin' which requires Dashboard permissions with that change. An issue we might have to look at anyway. For now, I've just changed all of those to admin/config.

Here's what I think makes the block tests fail:

During the tests, the "management" navigation block is moved around. With the given permission set ('administer blocks', 'administer filters' and 'access administration pages'), this block contains just a link to the Administer page. This patch however changes the admin path to point to the dashboard, which requires 'access dashboard' permission. Hence, the management block becomes empty since the user does not have permission to any of the items and it's no longer rendered causing the tests to fail.

Updated patch attached which now includes a small change to the block tests so the user has 'access dashboard' permission, and tests now pass on my local machine...

Cannot redeclare dashboard_menu_alter() - on line 36, is the testbot putting out

Sorry about that, the patch contained the dashboard_menu_alter block twice.

This looks good, although the extra tabs confused me for a second. Plus, the tab titles didn't really make sense next to 'Dashboard'. Not problematic per se, but Bojhan and yoroy might have additional thoughts on this.

The screenshot includes some bonus comments in orange -- outside the scope of this patch.

Those tabs should be moved to /admin/index.. I am not sure if thats part of this patch or of #699848: admin/by-task is confusing since it lacks links to config pages and looks similar to admin/config and friends .

Off to RTBC, I agree with Dries - I think this should be solved in #699848: admin/by-task is confusing since it lacks links to config pages and looks similar to admin/config and friends

Created #715790: Apply 'empty state' pattern to dashboard's recent content block for the 'empty text' in the recent comments block.

Committed to CVS HEAD. Thanks!

No way. Absolutely. This patch needs to be reverted, and that's out of the question.

#719964: Are you kidding me?

I have spent HOURS to debug why a regular user does not see the Management menu *at all*.

Straight rollback.

You will have to be more articulate about why you feel this is so wrong. Lowering priority and putting this back in the standard review process.

1. You need to grant the "access dashboard" permission to ANY user that should be able to access the Management menu.
2. The permission "access administration pages" has NO effect at all, if Dashboard module is enabled.
3. There is absolutely no hint, no note, no documentation about this hack.
4. Major hook_menu_alter() implementations like this are only acceptable in contrib. Such modules can state that they BREAK 2,841 other contributed modules on their project page.

No RTBC-ing your own patch!

Except making the 'access administration pages' useless, this doesn't break any functionality out of the box. I'm lowering the priority again.

It completely breaks Administration menu.

And Toolbar. (actually counts for "out of the box")

Not sure I understand it, also not sure why we couldn't fix this without a rollback?

I believe the commit here also caused #721328: Dashboard link no longer appears in the toolbar

Thanks!

#35: dashboard.patch queued for re-testing.

Update: I couldn't believe this patch would cause drupalAlterTestCase() to fail, but there it is. Any idea why?

The patch changes the active theme from garland to seven. I'm not sure why and if that's really intended, but given that, it's pretty obvious why the tests fail. common_test.module implements the alter hook for Garland and because of that change, that hook isn't called anymore. Changing that to Seven fixes the test but again, I'm not sure if that's what we want.

Strange, does the test bot use a different theme?

Let's try a new approach then, hard-code garland as the active theme before calling drupal_alter().

That test appears to be fundamentally flawed - it is testing the theme system but doing so from the testing environment, so you can actually get different results depending on how you are running the test (e.g. in the overlay or outside).

The fix seems reasonable enough; I guess a more complete fix would probably involve moving all the drupal_alter() calls into their own environment (i.e., within the simpletest browser) like most theme tests, but that's not really what we're even trying to do with this patch :)

I rerolled with a minor fix to the code comment. This looks ready to go to me.

+    'theme callback' => 'variable_get',
+    'theme arguments' => array('admin_theme'),

This isn't necessary; the menu item already inherits this info from its parent.

(In what way did the patch "change the active theme from Seven to Garland"? I don't see anywhere it did that; maybe I missed something.)

If it's not necessary, then why do the tests pass with that change, and not without it?

They do pass without it - see Berdir's patch in #47 (and #48 too of course, the test failures on that were just the sporadic imagefield ones). Your patch in #50 contained the same changes to common.test that Berdir's did, and I think that's why it passed. If you can take the common.test changes out of your patch and still get the testbot to like it, then I'll be convinced :)

I admit I have no idea at all why the changes here are causing this test to fail (in the particular environment run by the testbot), but as mentioned, it's a fundamentally flawed test. To be more specific about what I said in #48, currently I experience (using Drupal HEAD with no patch applied) that those drupal_alter() tests fail when the simpletests are run in the overlay and pass when the simpletests are run outside the overlay! And then they pass in the overlay if I make Garland my admin theme and try running them. It is fundamentally broken and dependent on the testing environment in some kind of weird way.

Berdir's fix is not 100% perfect, but it at least moves in the right direction, in that it forces that test to run in a defined theme environment, which is what we want.

Yup, guess we'll see. Whatever is causing this though has something to do with the interaction between PIFR and Simpletest and the Batch API (it's ultimately the batch's theme that is getting used in that test - i.e. because the tests run as part of a batch process). That is probably the most complicated combination imaginable, and I definitely have no further will to debug it myself, although it would be interesting to understand :)

Should we open a follow-up so that it's written down somewhere that the test is flawed? Maybe someday someone is bored enough to debug that :)

Looks good, good code, no comments. Committed to CVS HEAD.

I think the original patch needs to be rolled back and the permission needs to go back in. Moving the dashboard to the main admin page causes a few issues:

- the admin toolbar is a list of items below /admin, so therefore will not list the dashboard as noted by David in #34 (we have a zillion issues submitted for that and they are richly interlinked)
- the other admin subitems will show up as tabs on the dashboard which will look odd and misleading as noted by Dries in #16
- the permission now removed makes the dashboard even harder to reuse outside of it doing anything but the main admin page

I think the issue started was not too elaborate on why would we put the dashboard as the main admin page, and the patch had too many side effects which we are trying to duct-tape around by removing the dashboard permission and then also attempting to hack around in the toolbar to get the dashboard displayed. Why not make it clean and put dashboard as a subitem under administration. We already have #699848: admin/by-task is confusing since it lacks links to config pages and looks similar to admin/config and friends to play around with reworking the existing admin pages.

Fully agreed.

Dashboard has not a single test? Wow.

Latest discussion that lead to the conclusion that it's better to revert this entirely happened in #596010: Move "Administration" link into Account menu and make Toolbar output first level of 'admin' menu

Thanks!

Looked over the patch, it looks good. Did not actually try though.

Not so quick :) I can see how this fixes things, and I'm not against removing dashboard as the admin homepage per se. But it's unclear to me what this new admin home looks like. Will review this.

Reading the arguments for this rollback are very strange, almost all of them seem technical bugs rather then UX decisions. Obviously that permissions are now causing problems is not good.

But the fact that Dashboard is only accessible by breadcrumb is because it somehow got removed from the top Toolbar. Also the secondary tabs, have already been put forward as a bug and we have been working on making that 1 tab (Index) #699848: admin/by-task is confusing since it lacks links to config pages and looks similar to admin/config and friends

So this solution is most definitely a wont fix, since the UX feature is for Dashboard to function as a home. That now all of a sudden the rest of Drupal's way doesn't work correctly, means we should fix that rather then re rolling to a worse UX.

Let me reiterate the current situation:

1. To give anyone access the dashboard, we need to give them "access administration pages" permission; this might give them access to other admin pages as well; ie. there might be no way to hand out dashboard access without letting the user loose on some admin pages (for a content editor who'd like to monitor some stuff but you'd not like to let her config stuff)

2. The dashboard item is not in the toolbar because the toolbar displays the special home button, items at admin/* and user links; now dashboard is at /admin, so it obviously is not displayed there

3. There are strange tabs on the dashboard, which have nothing to do with the dashboard.

If we are to roll back this patch, all these three are solved. The /admin page will still be able to house the admin index, and since the default admin navigation has no way to get to that page anyway except via backpedaling from the breadcrumbs in the overlay, it is a well hidden page anyway.

If we are not to roll back this patch, we have some major changes to do:

1. Review permissions all around core and do not use "access administration pages" permission for anything beyond the /admin page only. Split the "access main admin page" from "access other admin pages"? Not sure this will go down well in terms of understandability of the permission scopes. Anyway, we need to re-split the permissions for dashboard and other admin pages for the dashboard to be possibly accessible for non-superadmins. Otherwise the dashboard will have very limited use.

2. We can hack the toolbar to not only include / but also include /admin and then /admin/*. This is basically flattening three levels of links. Bojhan says this is more understandable compared to just listing /admin/* links as the toolbar was originally implemented. I don't think so, but we have no other options if we are not to roll this back.

3. Bojhan says via IRC that at least an "Index" tab will show on the dashboard even in the release (in replacement of by-module). I guess the by-task one will be totally overridden then if the dashboard is enabled and will not be accessible? How could people navigate their admin structure then if the toolbar is disabled but the dashboard is not? They'll have no way to navigate down to the subitems. Oh, yeah, there is the management menu, but Seven does not have columns, so you don't have a way to put in the management menu (and we'd need to put it there if the toolbar is not enabled but not put it there if the toolbar is enabled).

So in short, if this is not rolled back we need to (1) revise and re-split admin page permissions in a non-confusing way (2) display multiple levels of admin links in the toolbar (3) accept at least one other tab on the dashboard and (4) redesign the Seven theme to support sidebars and (5) implement magically appearing blocks based on toolbar module status.

This patch heavily correlates with the following issues:

#596010: Move "Administration" link into Account menu and make Toolbar output first level of 'admin' menu
#408160: Normal users should not see the create content link appear by default in a menu called "Management"
and also
#699848: admin/by-task is confusing since it lacks links to config pages and looks similar to admin/config and friends

It may be true that the dashboard and the structure of the Management menu was somehow imagined differently. But as of now and until now, the envisioned design doesn't map to our technical possibilities and is technically not doable without introducing major hacks.

To summarize all of those issues:

1) Dashboard gets a regular menu item on admin/dashboard, which makes it appear in the Management menu and in the toolbar.

2) The Administer menu link is moved into the Navigation menu, so users without Toolbar are able to access administrative pages. Sub-links below admin/* are kept in the Management menu, which also heavily simplifies Toolbar's code to retrieve the links to display. Therefore, all top-level links in the Management menu are displayed in the toolbar. (first issue linked above)

3) The "Add (new) content" link and sub-links are moved into the Navigation menu, so users without administrative permissions are able to create content. (second issue linked above)

4) Lastly, catch and others would like to improve and declutter the amount and output of administrative index pages, such as /admin and admin/config and admin/by-module. This issue is only related, because the dashboard replaced one of those index pages previously.

By moving forward here, we can immediately fix at least 2 other criticals, i.e. 3 in total.

#596010: Move "Administration" link into Account menu and make Toolbar output first level of 'admin' menu depends on this patch.

So here is a re-roll as I had a hunk fail.

But this sure feels like a fail, see attached. Every other item in the admin index page has a "sub link". Dashboard is completely empty.

@Scott: how is that a fail? This page lists all the subitems under the admin main page, and this page is slated to go away in favor of an index at #699848: admin/by-task is confusing since it lacks links to config pages and looks similar to admin/config and friends, so I'd consider this a temporary condition anyway.

Agreed with Gábor.

Sending to bot.

The dashboard container is missing
1.) hairline
2.) has no sub items
3.) both make it feel completely out of place.

Will given that the index page gets changed, it will be much better, as a 'temporary' fix ok.

@Sun

1) I would agree with if it didn't incidentally also mean that /admin would be the index, rather than the dashboard.

2) I could agree with this, but I would rather throw away Management then have *magic* admin navigation showing up. I am not sure why we are considering a complete menu overhaul this late, because in #273137: Split Navigation to User and Administration menu the case is made for separating Navigation from Administration duties, which to me still make sense.

3) I would agree, rather silly that it requires global administration permissions at all.

4) Doesn't apply?

@Gabor

1,3 ) Seem like bugs that need to be fixed, rather then reasons for this patch.
2 ) You are making it an technical decision, I don't care about 1 or 5 levels of links - I care about top navigation items making sense and removing Dashboard does not, it is the administrative home.

Anyways seems like I am the only advocate for having the Dashboard on the /admin level because Dashboard is the new administrative home. Conceptually I don't see why it wouldn't make sense - its how many many systems work. Sure technically it doesn't work right now, but do we need to bend UX to technical flaws or what? You can trow 200 technical reason at me (or just plain bugs), but I haven't heard a valid UX argument for not making Dashboard our administrative home.

Is what it looks like with #74 applied. At first it seems to make sense. But the 'administer' breadcrumb link is indeed where the fail happens. Click it and new users will get a site-map style link dump that will rival current D6 /admin in it's ability to confuse people. The UX strategy of *not* exposing everything at the same time goes flat on it's face here.

I wasn't too explicit in "Dashboard must be top level admin entrance" because it wasn't clear to me what it would be replaced with. As dashboard has it's top level link back with the latest patch, it seems to fix stuff. But if /admin becomes a site-map that's even larger than what we have on the configuration page then we indeed throw away a big part of the designed solution for 1 of the top-3 *known* usability issues, the one about 'not able to find functionality'. Especially because it's not linked to anywhere else so will likely be found through magic/accident, adding to the confusion.

Dashboard should remain the administrative home.

@Bojhan:

It's not only about technical reasons. It's also about an incomplete and invalid envisioned concept/idea of the administrative interface. To summarize, the current UX goals state exactly this:

A) The dashboard should appear on /admin, instead of the regular index page on /admin, if enabled.

B) The dashboard should appear as "Dashboard" link in the Management menu, whereas the Management menu contains links below admin/*.

Effectively, these statements contradict themselves.

In addition, Drupal core does not provide any technical facility to properly and cleanly do A), which was the reason for the partial rollback of the original patch in this issue.

Furthermore, Drupal core provides no technical means to do both A) and B), unless you are duplicating menu router items, which would lead to even worse consequences.

The current patches are not a "complete menu overhaul", but merely trying to make any sense of the envisioned administrative interface idea, by implementing what's possible in D7. Note that we are past the ~4th D7 code freeze.

Thus, to finally get rid of these critical bugs, we effectively move forward with B), as far as this issue is concerned.

@yoroy:

I really have to wonder how the dashboard helps in "finding" things...? The dashboard has nothing to do with finding administrative functionality - instead, it pretends to be a nice looking + customizable "administrative front page" that may allow to have a look at certain data of your site at a glance. If anything, then you might be able to see most recent content, statistics, and log messages there. But it does not allow to access administrative functionality.

Well, certain data is a positive here, not a negative. We only provide a very basic default here, that is true. Dashboard is very much an outcome of the ux strategy to privilege the content producer or, more general, provide a simpler admin page to collect day to day tasks on.
(Sites don't have their site map on the homepage either, right? )

Re: B, to be sure I understand: dashboard has to be in there as a fallback for if toolbar is disabled, right?

I understand the idea of the dashboard. But still, it does not provide access to administrative functionality. To overcome this, I guess the UX strategy envisioned that there would always be a toolbar, which provides that access. But the dashboard does not. And both the dashboard as well as the toolbar can be disabled, in any order, or not at all.

If Toolbar (and no other similar module) is not enabled, then users need to be able to both access administrative functionality, but may also want to access the dashboard (if enabled).

If Toolbar (or a similar module) is enabled, then a "Dashboard" link should show up in the toolbar (the Management menu), so as to allow to access it.

Sorry, but I entirely disagree with that thinking. Drupal is successful because of its modularity, and the community's strength to harness that power of modularity to create innovative solutions. If, by disabling a module that provides an administrative UI gimmick, Drupal becomes completely unusable, then we've lost a core value of Drupal.

I may have lost track of the problem we are trying to solve here, but as far as I can tell, this is nowhere near as serious as it's being made out to be. We have two main bugs that come out of this issue:

1. The dashboard link does not show up in the toolbar. This was #596010: Move "Administration" link into Account menu and make Toolbar output first level of 'admin' menu which as far as I'm concerned is a preexisting toolbar module bug that was exposed by this issue, but not actually caused by it. The toolbar module is hardcoding assumptions about the structure under /admin that don't make any sense (and never did), and we need to change those assumptions. I think we should reopen that and go back to fixing it there as we tried to do originally.

2. Second issue is the 'access administration pages' vs 'access dashboard' permission. If we stick with what we have in core right now, I'm not sure it's a disaster at all - 'access administration pages' isn't really used for any other functionality in core (just access to listing pages as far as I know), and even if it were, it's not a total disaster if the "content creator" needs that permission to see the dashboard. Going back to 'access dashboard' would make more sense, and it's not immediately clear to me why we can't make that work (I don't think I understand the original bug that was caused by this - maybe someone could explain more?). In the worst case, though, why can't we just have the dashboard module use a custom access callback such as user_access('access administration pages') || user_access('access dashboard'). That way anyone with 'access administration pages' could still see exactly what they could see before, but additionally, "content creators" (who might only have 'access dashboard') can still use the dashboard...

1. has already a working solution in #596010: Move "Administration" link into Account menu and make Toolbar output first level of 'admin' menu, which merely depends on this patch getting committed.

2. is also about a "Dashboard" link that should be exposed in the toolbar. The Administer link won't appear there (and should not).

@sun I do not see how they contradict themselves, it should serve as the new administration Home. I believe you wrongly assume that by placing another page there with an index of administration, we will help the user - we feel that this would rather confuse the user by telling them there is another place to look for functionality beyond Structure and Configuration.

Dashboard is not to serve as a functionality home, but rather a informational home.

I do think modularity in UX is our strength indeed, so I would never favour any approach that will kill UX in that space. However as you make it clear, the bad UX here will happen because of a faulty implementation. And no matter what freeze we are in, I do not feel a faulty implementation at first should be leading the mistakes we knowingly make in UX down the road.

ok, I think I've explained in detail how I'd resolve this entire bundle of critical issues in #72 and following comments. If that resolution does not work for any reason (which I still don't get), then I seriously but kindly ask for a complete definition, direction and explanation of how to resolve this bundle of issues, which takes all details of all these issues into account.

It is pretty hard to read up on the issue, but basically it appears to boil down to clean implementation vs UX Decision.

I was so pleased to find the dashboard at /admin. We still have too many "Linkdumps" like yoroy describes it, e.g. the configuration page will sadly become one with 20+ modules installed.

So if by any means the UX improvement prevails, please keep dashboard at /admin.
The dashboard itself has much improved over the months, and if we do not push people onto it with their nose, one can fear no one will ever use it - which would be a shame with all this slick drag- and droppability that puts block admin page to shame.

My comment will sure not help to solve the problem, but 2 Cents for overcoming technical implications in favor of making the dashboard rock.

subscribe

http://drupal.pastebin.com/nBhUJeHH

Dashboard and the By-module page are the two contenders for living at /admin.
By-task can go because the task-driven IA is expressed through dashboard/toolbar/configuration page now
So, lets rename 'by-module' to 'Index', it's a good name. Then:

With dashboard enabled, make it look like this, meaning dashboard lives at /admin.

Without dashboard, 'Index' becomes the fallback /admin:

Also, this issue needs cross-ref with #699848: admin/by-task is confusing since it lacks links to config pages and looks similar to admin/config and friends, where I posted these same two mockups because this needs to be seen as a whole.

Implementation-wise, we should take care to not make navigating admin impossible. The three scenarios from #91 (written up by webchick):

Three conditions:

1. Dashboard enabled, Toolbar enabled:

- "Dashboard" link shows up in Toolbar (first link after home icon)
- When clicking into Dashboard it has an "Index" tab which takes you to admin/by-task. This tab has two sub-tabs: "By task" / "By module".

2.Dashboard enabled, Toolbar disabled:

- Navigate to Dashboard through the "Management" menu. Same as above.

3. Dashboard disabled:

- /admin goes back to "Administer" for title, still is two tabs: "By task" and "By module". Now these are primary tabs rather than secondary.

__

The way I understand it, Dashboard and 'By-module/Index' should be siblings in top level of /admin. Somehow, with dashboard enabled, Dashboard gets to live on /admin itself. Without Dashboard, /by-module gets to live on /admin itself.

Incomplete reply this, but lets keep moving.

I'm lost. What is the decision that needs to be made here?

sun says in #86 that #596010: Move "Administration" link into Account menu and make Toolbar output first level of 'admin' menu fixes (part of) this. but 596010 is postponed waiting on this???

ummm. I'm sensing some circular waiting on something which is waiting on this type of logic here.

One of mistakes is using same blocks for dashboard and elsewhere. This leads to inconvenience like #849670: Recent content block usability

@andypost You are totally off topic :P?

All we need is patches

Let's repurpose this issue for the technical fix we need to make the Dashboard a proper local task. The UI discussion in #699848: admin/by-task is confusing since it lacks links to config pages and looks similar to admin/config and friends concluded that we still need to make it a local task.

This should do the trick.

Thanks for renaming the issue to reflect the real goal :) Tested the patch and yes, it works as intended. This doesn't interfere with #596010: Move "Administration" link into Account menu and make Toolbar output first level of 'admin' menu ?

+++ modules/dashboard/dashboard.module
@@ -65,10 +65,10 @@ function dashboard_menu() {
+  // Make the dashboard the default local task on /admin, but keep it visible
+  // in the menu (especially the toolbar).
...
+  $items['admin/dashboard']['type'] = MENU_DEFAULT_LOCAL_TASK | MENU_NORMAL_ITEM;

Shouldn't this be MENU_VISIBLE_IN_TREE instead? We don't want it to appear in the breadcrumb, or do we?

39 critical left. Go review some!

@sun: it doesn't matter, because none of the links below Dashboard actually display as a page to the end-user, but yes, I think it make sense to display Dashboard in the breadcrumb.

includes/menu.inc means we need a chx or pwolanin sign-off here.

A summary a bit more descriptive than "This should do the trick." of what we're doing here would not go hurt anything, either. :)

+++ modules/dashboard/dashboard.module
@@ -65,10 +65,10 @@ function dashboard_menu() {
 function dashboard_menu_alter(&$items) {
...
-  $items['admin/dashboard']['type'] = MENU_DEFAULT_LOCAL_TASK;
+  $items['admin/dashboard']['type'] = MENU_DEFAULT_LOCAL_TASK | MENU_NORMAL_ITEM;

Neither a thumbs up nor a issue/patch summary justifies going back to "needs review", as this merely needs a sign-off from sub-system maintainers and a commit.

This patch changes menu_local_tasks(), the function that figures out and builds the list of tabs for a page (all levels, including action links), to look for the internal menu system constants that make up local tasks, instead of merely looking at the two MENU_DEFAULT_LOCAL_TASK and MENU_LOCAL_TASK constants directly (and only). This allows us to register a local task, but add the necessary bit flags to make the resulting menu link not only a local task, but also a MENU_NORMAL_ITEM, i.e., which on its own means MENU_VISIBLE_IN_BREADCRUMB (self-explanatory) and MENU_VISIBLE_IN_TREE, the very heart of this issue: Dashboard should still be rendered as regular menu link in the toolbar and other menu blocks.

Powered by Dreditor.

OK, I like this, I added some more comments. This leads me to believe we need a follow up with actions -- can an action be anything else but MENU_LOCAL_ACTION? If not then remove the bitmasking of it and just check for ==.

That was one issue more than asked for.

This leads me to believe we need a follow up with actions -- can an action be anything else but MENU_LOCAL_ACTION?

Actions are special types of local tasks. If we allow local tasks to be displayed in tree, there is no reason not to extend this to actions.

Except that visually we can't have actions living in a tree, contrib can though - so thats fine.

+++ includes/menu.inc	2010-08-03 04:31:48 +0000
@@ -1771,12 +1775,19 @@ function menu_local_tasks($level = 0) {
       foreach ($children[$path] as $item) {
+        // Local tasks can be normal items too, so bitmask with ¶
+        // MENU_DEFAULT_LOCAL_TASK before checking.
+        if (!($item['type'] & MENU_IS_LOCAL_TASK)) {

The constant in the comment is a copy+paste error, should be the same as in the following code line.

+++ includes/menu.inc	2010-08-03 04:31:48 +0000
@@ -1771,12 +1775,19 @@ function menu_local_tasks($level = 0) {
+          // The default task is always active. As tabs can be normal items
+          // too, so bitmask with MENU_DEFAULT_LOCAL_TASK before checking.
+          if (($item['type'] & MENU_DEFAULT_LOCAL_TASK) == MENU_DEFAULT_LOCAL_TASK) {
             // Find the first parent which is not a default local task or action.
-            for ($p = $item['tab_parent']; $tasks[$p]['type'] == MENU_DEFAULT_LOCAL_TASK; $p = $tasks[$p]['tab_parent']);
+            for ($p = $item['tab_parent']; ($tasks[$p]['type'] & MENU_LINKS_TO_PARENT) == MENU_LINKS_TO_PARENT; $p = $tasks[$p]['tab_parent']);

Shouldn't we bitmask with MENU_LINKS_TO_PARENT as well in the primary condition instead?

+++ includes/menu.inc	2010-08-03 04:31:48 +0000
@@ -1824,15 +1839,18 @@ function menu_local_tasks($level = 0) {
+          if (($item['type'] & MENU_DEFAULT_LOCAL_TASK) == MENU_DEFAULT_LOCAL_TASK) {
             // Find the first parent which is not a default local task.
...
+            for ($p = $item['tab_parent']; ($tasks[$p]['type'] & MENU_DEFAULT_LOCAL_TASK) == MENU_DEFAULT_LOCAL_TASK; $p = $tasks[$p]['tab_parent']);

@@ -2214,7 +2232,7 @@ function menu_get_active_breadcrumb() {
     // Don't show a link to the current page in the breadcrumb trail.
-    if ($item['href'] == $end['href'] || ($item['type'] == MENU_DEFAULT_LOCAL_TASK && $end['href'] != '<front>')) {
+    if ($item['href'] == $end['href'] || (($item['type'] & MENU_DEFAULT_LOCAL_TASK) == MENU_DEFAULT_LOCAL_TASK && $end['href'] != '<front>')) {

Ditto here.

Powered by Dreditor.

OK, we discussed this one a lot at the sprint. Everyone breathe deeply. There is no solution that will please everyone. Here is what we finally settled on:

1. dashboard now always appears at /admin/dashboard. we did not like that it changed path based on enabled modules. this also fixes the outstanding issue where dashboard does not appear on the toolbar.
2. we included the last last menu.inc patch so that dashboard appears as a local task on /admin. the task appears all the way to the right in order to keep the By Task and By Module tabs together.
3. /admin is unchanged. feel free to change that in another issue.

Dries reviewed this approach and approves.

To add some more to that, one of the things that convinced us (or at least convinced me) that we should do this is that it really isn't clear why Dashboard was at /admin to begin with?

Naturally we want the Dashboard to be a main focus of site administration, but we achieve that by putting it as the first item in the toolbar. Hijacking the /admin URL on top of that really only matters for people who navigate their site by typing in URLs manually :) Also, besides the various bugs, a side effect of having Dashboard hijack /admin was that it became the default root of all breadcrumbs, but that didn't make sense since breadcrumbs are supposed to be about navigation, but if you are on the Structure page and your breadcrumbs say "Dashboard >> Structure" then that implies if you click on Dashboard you'll be at a page where you can immediately navigate back to Structure... which the dashboard isn't.

BTW, when Moshe says "/admin is unchanged. feel free to change that in another issue..." the issue for that is #699848: admin/by-task is confusing since it lacks links to config pages and looks similar to admin/config and friends :)

As part of this patch can we also restore the 'access dashboard' permission?

This was removed in #57 above, but only to fix some of the side effects of having the dashboard take over /admin, so those aren't relevant anymore. And Gábor in #71 explains why it's a useful permission to have.

Also, besides the various bugs, a side effect of having Dashboard hijack /admin was that it became the default root of all breadcrumbs

No, this is not true since #99. We can *definitely* have /admin as the dashboard *and* keep the title of /admin as "Dashboard". This has been done for one month already :p

Damien, I'm not referring to the title of the breadcrumb; I'm referring to the place you go when you click on it.... I don't think #99 affects that. But in any case, #113 (which incorporates #99) certainly does :)

Reconsidering my point about the breadcrumbs a bit: Although it's true breadcrumbs are for navigation, it's also true they have a "Home" link, so to the extent that the dashboard is the administrative 'home' there is some logic to having a link to it appear there.

However, it's also true that the current setup of having it at /admin really is causing issues when we try to make it work cleanly, and confusion when the module is disabled, plus if we do #699848: admin/by-task is confusing since it lacks links to config pages and looks similar to admin/config and friends in concert then we'll have a clean, simple page at /admin (which won't 'compete' with the dashboard at all) which removes one of the main obstacles for doing this, and allows the Dashboard to work much more naturally with the rest of Drupal.

This one restores the 'access dashboard' perm per #115 and fixes the test failure.

Fix tests for perm change.

Looks good, but the parts like these look incorrect:

-    'access arguments' => array('access administration pages'),
+    'access arguments' => array('access dashboard', 'administer blocks'),
     'type' => MENU_CALLBACK,
   );
   $items['admin/dashboard/drawer'] = array(
     'page callback' => 'dashboard_show_disabled',
-    'access arguments' => array('administer blocks'),
+    'access arguments' => array('access dashboard', 'administer blocks'),

I think it just needs to be a find/replace for 'access administration pages'. The 'administer blocks' permissions shouldn't need to change at all.

***

It occurs to me we don't need to get too caught up on breadcrumbs, because if we needed to we could still make them behave however we want by having the Dashboard module alter them - overlay already does this: http://api.drupal.org/api/function/overlay_preprocess_page/7

So the breadcrumbs can be made to point wherever UX considerations dictate, even without the dashboard living at /admin.

ok, replaced perm per david's suggestion.

no idea what all those exceptions are about. unrelated to this patch i think.

Please be careful with making sprint consensus its very easy to leave UX standards hanging, without anyone its expertise on that field in the room.

Either way the main issue wasn't about URL browsing, but breadcrumb browsing, given that no one actually uses breadcrumb browsing - I am oke with this direction, if it means we can avoid tabs on Dashboard ( since that was a bigger trade off). Its a suboptimal solution at best, but I am totally done fighting crtical issues in the queue, its a completely dreadful experience.

Lets fix those fails! :)

woops. i had lost hook_perm(). this should pass.

I've pinged pwolanin and asked him to have a quick look at the proposed menu system changes.

Incorporating my feedback from #111. Key menu system functions have to use the internal menu constants. Not doing so was and still is the cause for major menu system bugs.

Can someone explain what this patch actually does now?

It seems somehow that we are trying to over-use the router table for navigation.

+++ modules/dashboard/dashboard.module	18 Aug 2010 13:09:07 -0000
@@ -29,22 +29,23 @@ function dashboard_menu() {
     'title' => 'Dashboard',
     'description' => 'View and customize your dashboard',
     'page callback' => 'dashboard_admin',
-    'access arguments' => array('access administration pages'),
+    'access arguments' => array('access dashboard'),
+    'type' => MENU_LOCAL_TASK | MENU_NORMAL_ITEM,

The essential change is this: We want Dashboard to appear both as menu link in menu trees, but also as local task on the path 'admin'.

That's a valid use-case within the current features of the menu system. There's no reason why the local task generation code should look for "higher" constants, if it really only cares for the most precise bits, i.e., whether a link is a local task or is a action.

EDIT: Also, the 'type' column actually rather belongs to menu links in reality. The menu router doesn't care for the type at all. So the usage for navigational purposes is just natural.

Powered by Dreditor.

Apparently nobody is actually *testing* this patch...

1) Got this chunk when I'm on the dashboard, tried with 3 different installs.

Notice: Trying to get property of non-object in user_access() (line 751 of C:\xampp\htdocs\drupal3\modules\user\user.module).
Notice: Trying to get property of non-object in user_access() (line 763 of C:\xampp\htdocs\drupal3\modules\user\user.module).
Notice: Trying to get property of non-object in user_access() (line 764 of C:\xampp\htdocs\drupal3\modules\user\user.module).
Notice: Trying to get property of non-object in user_access() (line 770 of C:\xampp\htdocs\drupal3\modules\user\user.module).
Notice: Trying to get property of non-object in user_access() (line 773 of C:\xampp\htdocs\drupal3\modules\user\user.module).

doesn't sound related but it *has* to be related. (did a before-after test)

2) Customising dashboard is also broken with this patch.

Didn't test further, this needs to be fixed first.

Well, I think that issue (or at least the first one) was fixed after my review in #123, no? But looking at the couple more recent patches it seems one more of those instances slipped back in:

-    'access arguments' => array('administer blocks'),
+    'access arguments' => array('access dashboard', 'administer blocks'),

You can't pass two permissions to user_access(), only one :)

Anyway needs work :). I'll try the next patch.

meh.

This topic is about: "Fix dashboard as the default /admin local task"

If I click on administer I land on the by task tab. Actually the "by task" tab is first than the "by module" and last in the row is the "dashboard" tab.
Is this the desired behaviour? If this is the correct behaviour the issue title is wrong.
(Or I don't understand this issue)

Talked with Bohjan about this on irc. The current "patch" behaviour is how it should be. So I'm fine with this.
Customising dashboard is also working again.

Screenshot to visualize the changes.

thanks for the reroll

unrelated: so our most common 'access arguments' usage takes an array of only one item. classy.

wait for green before commit.

Fixed various coding style issues. Looks RTBC to me.

oopsie

Fixed typo.

Great. Committed to CVS HEAD. Thanks.