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:
- Install D8.
- Install File, but not Node.
- Go to /admin/content.
- Go to /admin/content/files.
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.
Comment | File | Size | Author |
---|---|---|---|
#65 | admin_content_without_node.png | 15.25 KB | Xano |
#65 | admin_content_with_node_and_list_builder.png | 15.06 KB | Xano |
#65 | admin_content_with_node_and_views.png | 22.75 KB | Xano |
#64 | drupal_2085571_64.patch | 15.64 KB | Xano |
#42 | interdiff.txt | 1.2 KB | Xano |
Comments
Comment #1
XanoA few reasons why we should change this:
Comment #2
XanoThis patch demonstrates the failure.
Comment #3
webchickChanging 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.
Comment #4
XanoI 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?
Comment #5
yoroy CreditAttribution: yoroy commentedI think that's https://drupal.org/usability-test-university-baltimore-community-solutions
Also, https://groups.drupal.org/node/12618
Comment #7
XanoComment #8
XanoThanks 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.
Comment #9
BerdirYes, 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.
Comment #10
XanoAdded related issues.
Comment #11
xjmComment #12
Bojhan CreditAttribution: Bojhan commentedI 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.
Comment #13
XanoThanks for sharing your thoughts. There are two scenarios here:
admin/content
shows the node listing and listings for other types of content should be displayed as tabs.Comment #14
BerdirWhy 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?
Comment #15
XanoWhat 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 belowadmin/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?
Comment #16
Bojhan CreditAttribution: Bojhan commented@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 ;)
Comment #17
XanoThis 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.
Comment #18
dawehnerWe 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.
Comment #19
Bojhan CreditAttribution: Bojhan commented@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.
Comment #20
XanoThis 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.
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.
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.
Comment #21
XanoThe patch moves the Files link at
admin/content/files
toadmin/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.Comment #22
XanoThis 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.
Comment #23
andypostAnd 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?
Comment #24
dawehnerNot sure, but I think the following bit of #2177041: Remove all implementations of hook_menu should solve it.
Comment #25
XanoI 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.
Comment #26
XanoThis patch implements the solution from #25.
Comment #28
XanoComment #29
XanoComment #30
BerdirI 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.
Comment #31
dawehnerThe classname is kinda okay but the documentation should really explain what we do.
Comment #32
XanoComment #33
dawehnerSorry 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).
Comment #34
XanoThis issue does not touch that, and the existing Views overrides continue working with this patch.
Comment #35
dawehnerOh I linked to the wrong issue. The other issue is in already, so we need a reroll.
Comment #36
XanoRe-roll.
Comment #37
BerdirThis 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.
Comment #38
XanoThis moves the local task to System. As we only override the route when Node is enabled, this should work.
Comment #39
dawehner#37 seems to require an additional test. (maybe just some nice little local task unit test).
Comment #40
XanoUse
LocalTaskIntegrationTest
.Comment #41
XanoSo testing System's tasks breaks, because
\Drupal\system\Plugin\Derivative\ThemeLocalTask
calls procedural code. We canComment #42
XanoComment #44
dawehnerSorry I could not resists aka want to have a little bit fun at the evening.
Comment #45
tim.plunkettO_o
Why not use a $this->container or something?
lololol
Comment #46
dawehnerEven I don't really like that.
Comment #47
Xano46: drupal-2085571-46.patch queued for re-testing.
Comment #48
XanoComment #49
tstoecklerIs there a "not" too much in this comment?
Comment #51
XanoComment #52
tstoecklerBack to RTBC. Interdiff looks good.
Comment #54
Xano51: drupal_2085571_51.patch queued for re-testing.
Comment #55
XanoComment #57
Xano51: drupal_2085571_51.patch queued for re-testing.
Comment #58
XanoThat looks like a malfunctioning testbot.
Comment #60
Xano51: drupal_2085571_51.patch queued for re-testing.
Comment #61
Xano...
Comment #62
catchComment #63
Xano51: drupal_2085571_51.patch queued for re-testing.
Comment #64
XanoRe-roll.
Comment #65
XanoAll 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.
Comment #66
XanoComment #67
dawehnerCool, thank you!
Comment #68
alexpottCommitted cd63520 and pushed to 8.x. Thanks!
Comment #70
YesCT CreditAttribution: YesCT commentedtest in this patch was added not in psr-4 location (I think)
#2286241: Move stray SystemLocalTasksTest to PSR-4 location