Problem/Motivation
While working on #2005166: Create simple file listing under admin/content/file I wanted to display file listing (admin/content/files) also in responsive menu. In order to achieve that I needed to change menu item type configuration from "Menu tab" to "Normal menu item". This successfully exposed this page in responsive menu, but removed tab as a result.
here is a screenshot which explains the problem:
Since we plan to use views for more and more core listings I assume that we want to support this without any altering.
Proposed resolution
Make 'type' a multivalue, not just a single value. Especially because d8 has a more decoupled API, its less problematic than in earlier (d7) times.
.
Remaining tasks
User interface changes
Expose configuration in UI?
API changes
Related Issues
- #2005166: Create simple file listing under admin/content/file
Beta phase evaluation
Issue category | Task because it allows to improve UX for other core views and enables a little bit more sitebuilding. |
---|---|
Issue priority | Normal, because it affects just a limited amount of views. |
Disruption | The existing views configurations has to change from a single value to a multivalue field, so there is some disruption, but not a big one |
Comment | File | Size | Author |
---|---|---|---|
#137 | interdiff_136-137.txt | 2.17 KB | nikitagupta |
#137 | 2027043-137.patch | 34.53 KB | nikitagupta |
Comments
Comment #1
dawehnerStarted to work on it.
Comment #3
jibranTagging
Comment #4
pwolanin CreditAttribution: pwolanin commentedThe title/description is outdated since these need to be plugins now, not use hook_menu.
Comment #5
dawehnerWell, until we haven't converted views to use the new plugins (which needs the new method on the url generator you don't want to have).
So before being able to convert anything we have to remove getPath again from the local managers.
Comment #6
klonosRelated/depending on this one: #2047893: Add menu item(s) for the new admin/content/file view
Comment #7
dawehnerThis also converts the existing views configurations.
Comment #9
tstoecklerI think in general we try to avoid repetitive config such as "normal: normal" I think that should be either
"types:
- normal"
or
"types:
normal: 1"
depending on whether something like
"types:
normal: 0"
makes sense.
Comment #10
dawehnerI totally agree with the point made by tobias.
Comment #11
tim.plunkettSee #2049585: Views page displays with placeholders are forced to be MENU_CALLBACK also
Comment #12
tim.plunkettRerolled
Comment #14
tim.plunkettListing the files view as in the toolbar is also blocked by #2202493: views_menu_link_defaults() does not set a parent for links
Comment #15
tim.plunkettComment #16
tim.plunkettBecause there's a lot of indenting going on, here's a diff with no whitespace changes.
Comment #17
dawehnerI guess we should have some kind of integration test?
Comment #18
tim.plunkettAbsolutely.
If someone else wants to work on that, feel free. I might have time this weekend.
Comment #19
dawehnerSo looking forward to get some sleep this weekend. + adding a tag.
Comment #20
tim.plunkettLOL that's the tag I was looking for :)
Comment #21
dawehnerHaha
Comment #22
saltednutComment #23
saltednut15: vdc-2027043-15.patch queued for re-testing.
Comment #24
saltednutComment #26
saltednutWill attempt a reroll.
Comment #27
saltednutI'm not sure if a straight reroll can be done here. The foreach loop introduced in
Drupal\views\Plugin\views\display
seems to break since$menu['type']
is a string, not an array. Anyway, here is an attempt at this.Comment #28
saltednutComment #30
saltednut@tim.plunkett can you explain what the changes in
core/modules/views/lib/Drupal/views/Plugin/views/display/PathPluginBase.php
are intended to do? I am trying to reroll this but that is where I have issues. Doing a straight reroll causes PHP errors (see #27) but the OTHER changes that do apply toPathPluginBase
cause tests to fail. For now, what I am doing is rerolling all of #15 except for the changes toPathPluginBase
. Tests are passing locally with that, but there's likely something missing here that needs to be done forPathPluginBase
...Comment #32
saltednutOkay after some more review I think I realize what I was doing wrong. You really don't want to just check
!empty
but probably useis_array
instead - to ensure the$menu['type']
is something that can be iterated over.Comment #33
tim.plunkettCrosspost of sorts, I started this after I saw #30. We'll see how it fares in comparison.
Comment #36
saltednutDefault config has moved into
config/install
Also, we missed a view:
core/modules/user/config/install/views.view.user_admin_people.yml
Comment #37
saltednutOf course I'd post a patch with whitespace errors... >:)
Comment #38
saltednutPassing with testbot now but still needs test coverage per #17
Comment #39
mgiffordAs said above, needs "some kind of integration test".
Comment #42
Désiré CreditAttribution: Désiré commentedComment #43
Désiré CreditAttribution: Désiré commentedRerolled the patch. Seems to work.
Now I'll make some tests.
Comment #45
jibranHere is the reroll more later.
Comment #47
jibranNext step make it green.
Comment #49
jibranWith one assertion.
Comment #50
jibranSome minors.
deprecated.
more then 80 chars.
Comment #51
jibranFixed #50 and added some more asserts.
Comment #52
dawehner.
Comment #53
jibranblah :/
Comment #54
dawehnerJust curious, is this really the proper way to define a sequence? It seems wrong to have key: key pairs here. Let's see what vijaycs85 thinks about it
Comment #55
vijaycs85#54 ideally we want to have it like:
However we have few places in core where we have same value as both key and value. So yeah, would be great, if we can avoid key.
Comment #56
dawehnerThis is not really a feature but more like a task, given that people want to put stuff into the toolbar as well as into local tasks.
@vijaycs85
Would you be okay with getting it in as it is and make a bigger issue which cleans up all kind of various places we still do it?
Comment #57
vijaycs85Sure, I remember we fixed this in few places initially (when converted variable_get to config).
Comment #58
dawehnerOkay, cool
Comment #61
olli CreditAttribution: olli commentedManually tested the patch trying to add files view to toolbar and it works!! Two questions:
Isn't this same as
if (in_array('tab', $menu['type']))
?Can we drop the 'No menu entry' option now?
Comment #62
blackra CreditAttribution: blackra commentedI agree with Olli's point #2 in comment #61: having a 'No menu entry' checkbox no longer makes sense.
Other issues I have noticed are:
The UI says 'Type' above the checkboxes. This should be plural now that we are allowing multiple options. 'Menu Link Types' would make more sense.
I have also found what appears to be a bug. If you try to set a default menu tab then saving fails with 'Display Page is set to use a parent menu but the parent menu link text is not set'. I suspect this is because in the radio-button version of the UI, selecting the 'default menu tab' option caused the parent link option to disappear. There is some UI logic here that needs re-thinking.
Comment #63
olli CreditAttribution: olli commentedOh, I see, #61.1 is not same because for now the menu[type] is not filtered.
Similarly here the $menu_types is not filtered so these in_array()s are always true, right?
That empty() in the middle might be the cause for the second issue in #62.
That sounds like there is some #states conditions that should be updated.
Comment #64
jibranHere is a reroll and fixes for #61 and #63. Thanks @olli awesome observation.
Comment #65
jibranSorry about that :(. I hope it'll not fail because of this.
I have re-installed my local D8 and it is still removing these. I have no idea why it is doing this after #2349553: Store entity field information in the views data.
Comment #67
jibranWith the test fix.
Comment #69
jibran:/
Comment #71
dawehner@jibran
Is the last one on purpose?
Comment #72
jibranNo as you can see in the interdiff menu_names are admin and tools this should pass but I was unable to debug it locally. This fails locally as well can you please have a look at it?
Comment #73
olli CreditAttribution: olli commentedThis fixes the test. Adjusted #states also.
Comment #74
olli CreditAttribution: olli commentedComment #75
olli CreditAttribution: olli commentedtab_options type can still be 'none'.
Comment #76
jibranAwesome @olli thanks for fixing this.
Comment #77
dawehnerNo, config actually will, but let's just drop the comment, its not that helpful.
Comment #78
alexpottThis issue is a normal task so we need to outline how it fits within the allowable Drupal 8 beta criteria. Can someone add Drupal 8 beta phase evaluation template to the issue summary.
Also the issue summary has incomplete sections.
Comment #79
dawehnerAdded ISBE.
Comment #80
jibranBack to RTBC.
Comment #83
jibranBack to RTBC.
Comment #84
xjmTBH this sounds like a feature request to me, and the stated usability goal is a little thin. We can always say that new features "add usability".
Comment #85
alexpottThis foreach looks weird because the $type in not used.
Also considering we don't have any usages of this new functionality in core I think this must be considered a feature. The problem is this feature would be only possible to implement in Drupal 9 due to the API changes.
Also I disagree with the disruption evaluation won't all existing views with using the menu plugin break?
Comment #86
olli CreditAttribution: olli commentedFixed #85 by removing that foreach.
Comment #87
jibran@dawehner should we move it to D9 then?
Comment #88
xjmYeah, postponing to 9.x per #84 and #85. Thanks everyone for the work here and sorry this one didn't make it in before the beta.
If we can find a way to implement it in a BC way, we could do this in 8.1.x.
Comment #89
jibranWell I knew your intentions that's why I specifically asked @dawehner. :)
For me not able to access an admin link(admin/content/files) especially admin listing directly from menu/toolbar(click admin/content first then click files tabs) is a UX regression. In #1986606: Convert the comments administration screen to a view we fixed this issue by keeping the original menu item. It can also be fixed the same way here but having an option in views_ui is a UX and DX win.
I'll also classify it as major because it'll be a huge improvement for contirb modules(commerce, rules, entityforms, OG, media and etc.) admin listing UX.
I know we can live without fixing it because we have a workaround for this available.
Comment #90
dawehnerAlright, let's move it to be a bug, which can be solved in 8.0.x
Here is a screenshot which explains the bug as it is. You don't have Files as part of the responsive menu, why, because the Drupal 8 local tasks are decoupled
from menus, so when you can just do one of them at a time, you have to decide even technically they allow you to do both.
Comment #91
dawehnerThis time with a screenshot
Comment #92
dawehnerFixing this would certainly improve usablity
Comment #93
dawehnerWhat about adding a BC layer so that existing views don't break or as alternative in Page.php convert the values automatically on
::init()
time ...Comment #94
jibranHere is a screenshot after enabling admin_toolbar.
Comment #95
olli CreditAttribution: olli commentedReroll
Comment #96
dawehnerComment #98
olli CreditAttribution: olli commentedComment #99
dawehnerAlright, jibran, xjm and dawehner talked about that issue on IRC.
Yes, technically being able to define multiple menu types in the views UI is more like a feature request. For core we can easily introduce the necessary .links.menu.yml
On the other hand xjm agreed to move it to 8.1.x and write an update function to fix the existing views.
Does someone mind to create an issue to fix the missing
.links.menu.yml
file for/admin/content/files
?Comment #100
jibranWe already have a related issue #2047893: Add menu item(s) for the new admin/content/file view
Comment #101
slashrsm CreditAttribution: slashrsm commentedComment #102
slashrsm CreditAttribution: slashrsm commentedComment #103
BerdirIt's not just about the admin/content/files use case.
Try to create the a view with 3 displays on a non-existing page like /whatever, /whatever/tab1 and /whatever/tab2 where tab1 and tab2 are local tasks for /whatever and that is a menu link somewhere.
That's currently very hard and confusing (with options that make no sense like a menu for the local tasks) and we (Crell and me) haven't found a way to actually make the menu link work.. the parent menu link stuff doesn't seem to work at all.
IMHO, this is a bug :)
Comment #104
Crell CreditAttribution: Crell at Palantir.net commentedFYI, while trying to work around this gap in Views I ran into this bug: #2532490: Unrouted URLs break toolbar but are hidden by caching.
One way or another (or both) we need to address this before 8.0.0.
Comment #105
dawehnerI can't agree more. Its a bug users expect to be able to configure, then they can't configure it and are confused. It is a usability bug for example.
Something which was possible and is actually a REALLY common usecase. Just take the example that its sort of a small "blocker" for contrib. You want people to allow to disable the view, without having a local task plugin hanging around, as you need to define it manually.
Let's at least categorize this as a bug.
Comment #106
jibranBug for 8.1.x doesn't make sense to me.
Comment #107
Crell CreditAttribution: Crell at Palantir.net commentedI concur. Especially if right now the alternative leads to a fatal site. :-)
Comment #108
dawehner@Crell
Oh wait, does that issue fixes the other issue?
Comment #109
olli CreditAttribution: olli commentedrebase
Comment #111
Crell CreditAttribution: Crell at Palantir.net commenteddawehner: I don't think it would fix it directly. Rather, I ran into the other issue while trying to work around this bug. I will try to test later today to see if that being fixed makes it possible to do what I'm trying to do.
Comment #112
xjmThis issue was marked as a beta target for the 8.0.x beta, but is not applicable as an 8.1.x beta target, so untagging.
It is disruptive, so by default should be targeted for 8.2.x at this point.
Comment #113
Gábor HojtsyShould this now use the new capability and define it as a menu item as well? Found while reviewing with @tamas.gergocs
This change dose not seem to be backwards compatible and therefore I am not sure this is allowed in Drupal 8.x. A new key can be introduced with an update path to work around this limitation.
Comment #115
Gábor HojtsyComment #118
vijaycs85There are so some issues(#2047893: Add menu item(s) for the new admin/content/file view for file, #2906496: Give Media a menu item under Content for media) coming up and getting blocked because of this issue in core. Surely there will be plenty on contrib as well. Could we pump the priority to move this forward?
Comment #119
dawehnerComment #120
jofitz CreditAttribution: jofitz at ComputerMinds commentedRe-rolled.
Comment #122
jofitz CreditAttribution: jofitz at ComputerMinds commentedAddress some of the test failures.
Comment #124
jofitz CreditAttribution: jofitz at ComputerMinds commentedCorrect error in re-roll.
Comment #126
jofitz CreditAttribution: jofitz at ComputerMinds commentedFound and corrected a couple more re-roll errors and fixed 2 coding standards errors.
Comment #134
tea.time CreditAttribution: tea.time commentedIt's been awhile since any activity on this, but wanted to throw in a +1. I just ran into it myself and found it baffling that I have to choose between placing my view page in the menu and showing it as a tab. Like many of the core admin pages are, I'd like it to be both. :)
FWIW I'm thinking a possible workaround could be to set the "Normal menu entry" in the view and implement the tab ('local task') in a module: https://www.drupal.org/docs/drupal-apis/menu-api/providing-module-define...
... or vice versa, set the "Menu tab" in the view and implement the menu item in a module: https://www.drupal.org/docs/drupal-apis/menu-api/providing-module-define...
Comment #136
nikitagupta CreditAttribution: nikitagupta at Srijan | A Material+ Company for Drupal India Association commentedReroll patch #126.
Comment #137
nikitagupta CreditAttribution: nikitagupta at Srijan | A Material+ Company for Drupal India Association commentedworked on test case failures and deprecation.
Comment #138
andypostTests failed
Comment #140
SocialNicheGuru CreditAttribution: SocialNicheGuru commentedi get the following errors when I use drush to enable or disable modules.
in_array() expects parameter 2 to be array, null given ViewsLocalTask.php:121 [warning]
Invalid argument supplied for foreach() PathPluginBase.php:350
Several views have "Menu: No menu".
that is causing the issue for me.
Comment #141
catch