Problem/Motivation

/admin/content has become a centralized location for content listings that do not require their own section (such as users). However, the route and menu item are still located in node.module and in D7 or early D8 we made the node listing the default local task for that route, essentially giving other content listings the finger. The reason for this change was usability and that seeing another layer of menu items at admin/content was a bad idea. I understand this, but the current situation is even less usable, as there is no way to find admin/content/files through the menu when you disable node.module, for instance.
Steps to reproduce the problem:

  1. Install D8.
  2. Install File, but not Node.
  3. Go to /admin/content.
  4. Go to /admin/content/files.

Image 10-9-13 at 13.21 .png

Proposed resolution

Bring back the list of menu items at admin/content when Node is disabled, so the page is accessible and users can click on local tasks and child menu links instead of seeing a 404 page not found. See comment #65 for screenshots.

Remaining tasks

None.

User interface changes

None, apart from an accessible admin/content page instead of a 404 page not found.

API changes

None.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Xano’s picture

A few reasons why we should change this:

  1. Node.module is no longer required in Drupal 8. This is why we haven't run into this problem before.
  2. In Drupal 8 we clearly differentiate between content and configuration. There are two top-level menu items that mirror this: /admin/content and /admin/config. /admin/content, however, is hijacked by node.module.
Xano’s picture

Status: Active » Needs review
FileSize
698 bytes

This patch demonstrates the failure.

webchick’s picture

Changing from a content listing to a list of links would be an enormous UX regression and set us back to the "bad old days" in D6 where no one could find the "node orphanage".

What about moving admin/content to system module instead? Then the node view could override it.

Xano’s picture

What about moving admin/content to system module instead? Then the node view could override it.

I don't feel much for changing a menu's behavior based on whether a particular module is enabled or not. Also, we have a node list controller that will need a place in the menu when Views isn't enabled, which means Node will always have a content listing, no matter what.

The problem as I see it, is that we are so keen to keep Node.module on that pedestal we once put it on, that we keep having to come up with workarounds. Tabs are being abused at /admin/content, because we want Node content to show up first. Enable a few modules that provide content listings and you end up with two hands full of tabs that should never have been tabs in the first place.

Do you perhaps know where I can find the results of the study that discovered the problems with /admin/content in D6?

yoroy’s picture

I think that's https://drupal.org/usability-test-university-baltimore-community-solutions

Evaluators successfully added content but didn't recognize it had been created: Node administration

Also, https://groups.drupal.org/node/12618

Status: Needs review » Needs work

The last submitted patch, admin_content_is_broken_test_only.patch, failed testing.

Xano’s picture

Title: amin/content should not depend on node.module » admin/content should not depend on node.module
Xano’s picture

Issue summary: View changes

Thanks to this problem, I also discovered #2198571: menu.inc must not unset MenuLink::menu_name, and #2188097: "Comment forms" links to admin/content/comment suffers from this page belonging to Node instead of System as well.

Berdir’s picture

Yes, the problem is that with the new menu default links, this doesn't automatically fall back to /admin as it used to, but will throw notices (right now, could be fixed as the issue is suggesting) and links below it will not show up at all I think.

The comment issue in there now handles that with a manual fallback, but that's pretty ugly.

Xano’s picture

xjm’s picture

Issue tags: +VDC
Bojhan’s picture

I think in the first few tests on the content administration we concluded that having a top level was worth the effort. We can also conclude from the dozens of user tests since that content administration as a top link is working fine. This is one of our main UI's, probably the 2nd most used interface past the content creation screen. I see no reason to give up on UX or effectiveness, by bringing that to the Drupal 5 interface pattern.

I suggest instead to go a different technical direction and implement a more pluggable information architecture, where you don't need the node module to create the initial content listing and turn it into a list (like suggested) when there is no node.module but just turn it into the content listing with the files, etc. as tabs when the node.module is enabled. May sound hacky, but I think its a good way to preserve proven UX.

Xano’s picture

Thanks for sharing your thoughts. There are two scenarios here:

  1. Node is enabled. In this case, admin/content shows the node listing and listings for other types of content should be displayed as tabs.
  2. Node is disabled. We can technically still use the tabs, and it would be hacky indeed to convert those to regular links for a list, especially since in Drupal 8 menu links and local tasks are no longer in any way related. If we keep the tabs, what do we use as the main page contents?
Berdir’s picture

Why not simply go with something like "The Node module is not enabled"?

I mean, we're talking about *node* module. While it is now technically optional, 99% of the sites are still going to use it I expect and it's really not a thing for UX to worry about that case, people that disable node either know what they're doing and then they can always replace that page with something else or they did it accidently and then this might actually be a useful hint?

Xano’s picture

What I've done for Payment, because many users were not aware of or able to find the payment listing at admin/content/payment which was accessible through a local task, was add a menu link below admin/content, so it would show up below Content in the sidebar.

Maybe it's worth seeing if we should rethink the structure of this section altogether? I'm not saying it must be changed after all, but seeing as Files and Payment tabs are not really as local relative to node content as the name local tasks makes it seem, maybe these should have been menu links from the start?

Bojhan’s picture

@Berdir If we can do something like that I'd prefer it, because that doesn't affect the 99%. People who disable the node module either really know what they are doing, or have no freaking clue ;)

Xano’s picture

FileSize
26.33 KB


This is essentially what I said in #15: these are all local tasks, but what are they local to? The way this works now, we pretend these tasks are all for Node content, while they are certainly not. This becomes worse when contributed modules need to add listings of their own types of content.

// Edit: The root cause of this, as it appears to me, is that our concept of content has changed since we were developing Drupal 7 and those UX tests were performed. At that time nodes were the only content we had, and even if you include comments, those were only available to nodes anyway. Since then, we have come to consider many more data types content as well, such as files, in my cases payments, and users (which still have their own top-level menu item). While I agree that nodes are still the most important type of content, our situation has changed such that we cannot ignore that the UI may have to be updated to reflect these changes.

dawehner’s picture

We have similar problems all over in core when we have static "configuration" like a view with some fields, which just exists if language module is enabled.
For that we have introduced a 'provider' as well as an optional flag. Maybe we could think about 'optional' parent or something, but yeah this is no real solution.

Bojhan’s picture

@dawehner We do need a solution for when we have no parent. Its been a consistent problem over the years. Now that we basically place any entity under content. I prefer that people who do this create separate top level entries (like drupal commerce does), this it might not scale very well.

The most challenging part would be what you expose in the UI, the "empty parent" or do you switch it immediately to the first available tab? I think both options would be good enough to handle this edge case. I think its wrong to assume this problem will only occur in admin/content. So just handling this case, would not be beneficial we need a way to handle all cases where the parent has no contents.

I don't think it's fair to question the research, although its easy to do so (since I do research myself, I know how easy it is to do). We do research at a ad-hoc volunteer basis, but have since the D6 test that uncovered this issue tested the content creation and content listing dozens of times with no issues occurring around this part. Although its true we house more and more things here that doesn't strictly require node.module to be enabled - this itself doesn't invalidate the research (in which we always try to target the 80% usecases).

I'd like this discussion to be about finding solutions, and less about proving the need for this in the first place - because I feel that is already widely understood.

Xano’s picture

I prefer that people who do this create separate top level entries (like drupal commerce does)

This would be easily done technically. We do need to consider the fact that even in core this would mean two extra top-level menu links (Comments and Files), and that contrib may add some more. From a UX and design point of view, are our current menu display tools, such as the toolbar, able to handle this?
Moving these links to the top level would also remove the need to ask–and answer–the question What are these tabs local to?, since they will no longer be local tasks.

The most challenging part would be what you expose in the UI, the "empty parent" or do you switch it immediately to the first available tab? I think both options would be good enough to handle this edge case. I think its wrong to assume this problem will only occur in admin/content. So just handling this case, would not be beneficial we need a way to handle all cases where the parent has no contents.

There has been some thought about this, but no consensus as far as I know. I have not been able to think of any disadvantages making a menu link top-level if its defined parent does not exist, except that on different sites, importing the same menu links can have different results. We already allow menu links to be re-parented from the UI, so there should be no technical limitation here.

I don't think it's fair to question the research

Please know that I absolutely did not question the research. The point I was trying to bring across is that it's indeed very important for users to be able to find the node listing quickly and easily, as the research proved, but that the solution to achieve this that was adopted for Drupal 7, may no longer work as well for Drupal 8 anymore, because so much has changed. In short: the same problem may need a different solution in different Drupal versions.
I hope I have been able to clear up this misunderstanding.

Xano’s picture

Status: Needs work » Needs review

The patch moves the Files link at admin/content/files to admin/content. It doesn't show up in the menu right away, because of #2202493: views_menu_link_defaults() does not set a parent for links.

Xano’s picture

This is what it looks like if we let modules add top-level items. I haven't looked into the possibility of letting them provide icons yet, though.

andypost’s picture

And at the same time we buried custom block library at admin/structure/block/custom-blocks
But they are content!
How the content editors should edit blocks? or we need to allow them access to place blocks?

dawehner’s picture

Not sure, but I think the following bit of #2177041: Remove all implementations of hook_menu should solve it.

 function custom_block_menu_link_defaults() {
@@ -92,6 +46,11 @@ function custom_block_menu_link_defaults() {
     'description' => 'Add custom block',
     'route_name' => 'custom_block.add_page',
   );
+  $items['custom_block.list'] = array(
+    'link_title' => 'Custom block library',
+    'parent' => 'block.admin.structure',
+    'description' => 'Manage custom blocks.',
+  );
 
   return $links;
 }
Xano’s picture

I just had a chat with @dawehner and @pwolanin and we concluded that we can move admin/content to System and make it act like it did in Drupal 6. We can then let Node override the route and set the node list as the controller, just like Views overrides the route now. We will not lose in functionality, but admin/content will then also be available to modules if Node is disabled.

Xano’s picture

This patch implements the solution from #25.

Status: Needs review » Needs work

The last submitted patch, 26: drupal_2085571_26.patch, failed testing.

Xano’s picture

FileSize
10.39 KB
1.3 KB
Xano’s picture

Status: Needs work » Needs review
Berdir’s picture

+++ b/core/modules/node/lib/Drupal/node/Routing/RouteSubscriber.php
@@ -0,0 +1,34 @@
+/**
+ * Listens to the dynamic route events.
+ */
+class RouteSubscriber extends RouteSubscriberBase {

I know we have this as well in other modules, but both the description and the class name is very generic, we currently have three RouteSubscriber classes.

I think elsewhere we somehow name event subscribers after that they do?

Looks good to me otherwise.

dawehner’s picture

The classname is kinda okay but the documentation should really explain what we do.

Xano’s picture

FileSize
10.45 KB
797 bytes
dawehner’s picture

Sorry I would review it, if this would not break #2228207: Test reset menu links so hard.

In general the question is whether we should document how views can still take over the page (different priority).

Xano’s picture

In general the question is whether we should document how views can still take over the page (different priority).

This issue does not touch that, and the existing Views overrides continue working with this patch.

dawehner’s picture

Status: Needs review » Needs work

Oh I linked to the wrong issue. The other issue is in already, so we need a reroll.

Xano’s picture

Status: Needs work » Needs review
FileSize
10.57 KB

Re-roll.

Berdir’s picture

Status: Needs review » Needs work

This doesn't really work yet:

- Install minimal
- Remove node
- Go to /admin/content => No administrative items (administrative, huh? ;))
- Enable comment
- Same. Comment isn't shown in the list because it is a local task and there is no default local task so local tasks aren't displayed.

Xano’s picture

Status: Needs work » Needs review
FileSize
835 bytes
11.01 KB

This moves the local task to System. As we only override the route when Node is enabled, this should work.

dawehner’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

#37 seems to require an additional test. (maybe just some nice little local task unit test).

Xano’s picture

Use LocalTaskIntegrationTest.

Xano’s picture

So testing System's tasks breaks, because \Drupal\system\Plugin\Derivative\ThemeLocalTask calls procedural code. We can

  1. Not test System's local tasks and just get this issue fixed without a test right now. Considering the nature of the possible test coverage, I would be okay with that.
  2. Postpone this issue on a new issue that will be about testing System's local tasks, so we can build on that afterwards for this issue.
Xano’s picture

Status: Needs work » Needs review
FileSize
12.39 KB
1.2 KB

Status: Needs review » Needs work

The last submitted patch, 42: drupal_2085571_42.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
15.58 KB
4.45 KB

Sorry I could not resists aka want to have a little bit fun at the evening.

tim.plunkett’s picture

  1. +++ b/core/modules/system/tests/Drupal/system/Tests/Menu/SystemLocalTasksTest.php
    @@ -25,11 +36,26 @@ public static function getInfo() {
    +    $container = parent::setUp();
    
    +++ b/core/tests/Drupal/Tests/Core/Menu/LocalTaskIntegrationTest.php
    @@ -44,6 +44,7 @@ protected function setUp() {
    +    return $container;
    

    O_o

    Why not use a $this->container or something?

  2. +++ b/core/modules/system/tests/Drupal/system/Tests/Menu/SystemLocalTasksTest.php
    @@ -25,11 +36,26 @@ public static function getInfo() {
    +    $theme = new Extension('theme', '../../../../../../../themes/bartik', 'bartik.info.yml');
    

    lololol

dawehner’s picture

Even I don't really like that.

Xano’s picture

46: drupal-2085571-46.patch queued for re-testing.

Xano’s picture

Status: Needs review » Reviewed & tested by the community
tstoeckler’s picture

+++ b/core/modules/node/lib/Drupal/node/Routing/RouteSubscriber.php
@@ -0,0 +1,37 @@
+    // As nodes are the primary type of content, the node listing should be
+    // easily available. In order to do that, override admin/content to not show
+    // a node listing instead of its child links.

Is there a "not" too much in this comment?

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 46: drupal-2085571-46.patch, failed testing.

Xano’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
1007 bytes
15.83 KB
tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC. Interdiff looks good.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 51: drupal_2085571_51.patch, failed testing.

Xano’s picture

Status: Needs work » Needs review

51: drupal_2085571_51.patch queued for re-testing.

Xano’s picture

Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 51: drupal_2085571_51.patch, failed testing.

Xano’s picture

Status: Needs work » Needs review

51: drupal_2085571_51.patch queued for re-testing.

Xano’s picture

Status: Needs review » Reviewed & tested by the community

That looks like a malfunctioning testbot.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 51: drupal_2085571_51.patch, failed testing.

Xano’s picture

Status: Needs work » Needs review

51: drupal_2085571_51.patch queued for re-testing.

Xano’s picture

Status: Needs review » Reviewed & tested by the community

...

catch’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs screenshots
Xano’s picture

51: drupal_2085571_51.patch queued for re-testing.

Xano’s picture

FileSize
15.64 KB

Re-roll.

Xano’s picture

All screenshots are of admin/content.

Node and Views enabled

This is the node/content view, and it is identical both with and without the patch applied.

Node enabled and Views disabled

This is the node list builder, and it is identical both with and without the patch applied.

Node and Views disabled

This is a list of child menu links (or the message you see here when there are no child links). This situation gives a 404 page not found in HEAD, and shows this page with the patch applied, which allows users to access local tasks and child links even when Node is disabled. The red error is just Drupal whining the version is no longer supported and it has nothing to do with this issue.

Xano’s picture

Issue summary: View changes
dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Cool, thank you!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed cd63520 and pushed to 8.x. Thanks!

  • Commit cd63520 on 8.x by alexpott:
    Issue #2085571 by Xano, dawehner: Fixed admin/content should not depend...
YesCT’s picture

test in this patch was added not in psr-4 location (I think)
#2286241: Move stray SystemLocalTasksTest to PSR-4 location

Status: Fixed » Closed (fixed)

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