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

- #2005166: Create simple file listing under admin/content/file

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
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
CommentFileSizeAuthor
#137 interdiff_136-137.txt2.17 KBnikitagupta
#137 2027043-137.patch34.53 KBnikitagupta
#136 2027043-136.patch33.46 KBnikitagupta
#126 2027043-126.patch34.26 KBjofitz
#126 interdiff-124-126.txt2.2 KBjofitz
#124 2027043-124.patch34.13 KBjofitz
#124 interdiff-122-124.txt830 bytesjofitz
#122 2027043-122.patch34.14 KBjofitz
#122 interdiff-120-122.txt3.94 KBjofitz
#120 2027043-120.patch30.2 KBjofitz
#109 2027043-109.patch30.67 KBolli
#98 interdiff.txt622 bytesolli
#98 2027043-98.patch30.68 KBolli
#95 2027043-95.patch30.07 KBolli
#94 Screenshot from 2015-06-07 01:49:07.png22.42 KBjibran
#91 Screen Shot 2015-06-06 at 22.28.22.png80.22 KBdawehner
#86 interdiff.txt2.66 KBolli
#86 2027043-86.patch29.98 KBolli
#75 interdiff.txt831 bytesolli
#75 2027043-75.patch32.46 KBolli
#74 2027043-74.patch32.58 KBolli
#74 interdiff.txt1.97 KBolli
#73 interdiff.txt5.7 KBolli
#73 2027043-73.patch34.28 KBolli
#69 2027043-69.patch28.84 KBjibran
#69 interdiff.txt1.17 KBjibran
#67 2027043-67.patch28.82 KBjibran
#67 interdiff.txt22.72 KBjibran
#64 2027043-64.patch46.29 KBjibran
#64 interdiff.txt6.88 KBjibran
#53 vdc-2027043-52.patch26.24 KBjibran
#53 interdiff.txt904 bytesjibran
#51 vdc-2027043-51.patch26.25 KBjibran
#51 interdiff.txt3.97 KBjibran
#49 vdc-2027043-49.patch25.41 KBjibran
#49 interdiff.txt1.88 KBjibran
#47 vdc-2027043-47.patch24.62 KBjibran
#47 interdiff.txt1.8 KBjibran
#45 vdc-2027043-45.patch23.26 KBjibran
#43 vdc-2027043-43.patch23.27 KBDésiré
#37 vdc-2027043-37.patch22.7 KBsaltednut
#37 interdiff.txt497 bytessaltednut
#36 vdc-2027043-36.patch22.7 KBsaltednut
#33 vdc-2027043-33.patch22.57 KBtim.plunkett
#32 vdc-2027043-32.patch22.58 KBsaltednut
#32 interdiff.txt3.27 KBsaltednut
#30 vdc-2027043-30.patch19.31 KBsaltednut
#27 vdc-2027043-27.patch20.73 KBsaltednut
#16 review.txt18.1 KBtim.plunkett
#15 interdiff.txt9.72 KBtim.plunkett
#15 vdc-2027043-15.patch22.58 KBtim.plunkett
#12 interdiff.txt4.1 KBtim.plunkett
#12 vdc-2027043-12.patch17.08 KBtim.plunkett
#7 vdc-2027043-7.patch12.8 KBdawehner
#7 interdiff.txt5.48 KBdawehner
#1 vdc-2027043-1.patch8.52 KBdawehner
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

Status: Active » Needs review
FileSize
8.52 KB

Started to work on it.

Status: Needs review » Needs work

The last submitted patch, vdc-2027043-1.patch, failed testing.

jibran’s picture

Issue tags: +VDC

Tagging

pwolanin’s picture

The title/description is outdated since these need to be plugins now, not use hook_menu.

dawehner’s picture

Well, 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.

klonos’s picture

dawehner’s picture

Status: Needs work » Needs review
FileSize
5.48 KB
12.8 KB

This also converts the existing views configurations.

Status: Needs review » Needs work

The last submitted patch, vdc-2027043-7.patch, failed testing.

tstoeckler’s picture

I 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.

dawehner’s picture

I totally agree with the point made by tobias.

tim.plunkett’s picture

tim.plunkett’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: +beta target
FileSize
17.08 KB
4.1 KB

Rerolled

Status: Needs review » Needs work

The last submitted patch, 12: vdc-2027043-12.patch, failed testing.

tim.plunkett’s picture

Listing the files view as in the toolbar is also blocked by #2202493: views_menu_link_defaults() does not set a parent for links

tim.plunkett’s picture

Title: Support page displays to be marked as menu items of MENU_LOCAL_TASK | MENU_NORMAL_ITEM » Allow page displays to be both local tasks and menu items
Status: Needs work » Needs review
FileSize
22.58 KB
9.72 KB
tim.plunkett’s picture

FileSize
18.1 KB

Because there's a lot of indenting going on, here's a diff with no whitespace changes.

dawehner’s picture

Status: Needs review » Needs work

I guess we should have some kind of integration test?

tim.plunkett’s picture

Issue tags: +Needs work

Absolutely.
If someone else wants to work on that, feel free. I might have time this weekend.

dawehner’s picture

Issue tags: +Needs tests

So looking forward to get some sleep this weekend. + adding a tag.

tim.plunkett’s picture

Issue tags: -Needs work

LOL that's the tag I was looking for :)

dawehner’s picture

Haha

saltednut’s picture

Issue tags: +Media Initiative
saltednut’s picture

15: vdc-2027043-15.patch queued for re-testing.

saltednut’s picture

The last submitted patch, 15: vdc-2027043-15.patch, failed testing.

saltednut’s picture

Assigned: Unassigned » saltednut
Issue tags: +Needs reroll
error: patch failed: core/modules/views/lib/Drupal/views/Plugin/views/display/PathPluginBase.php:299
error: core/modules/views/lib/Drupal/views/Plugin/views/display/PathPluginBase.php: patch does not apply

Will attempt a reroll.

saltednut’s picture

Status: Needs work » Needs review
FileSize
20.73 KB

I'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.

saltednut’s picture

Assigned: saltednut » Unassigned

Status: Needs review » Needs work

The last submitted patch, 27: vdc-2027043-27.patch, failed testing.

saltednut’s picture

Status: Needs work » Needs review
FileSize
19.31 KB

@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 to PathPluginBase cause tests to fail. For now, what I am doing is rerolling all of #15 except for the changes to PathPluginBase. Tests are passing locally with that, but there's likely something missing here that needs to be done for PathPluginBase...

Status: Needs review » Needs work

The last submitted patch, 30: vdc-2027043-30.patch, failed testing.

saltednut’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
3.27 KB
22.58 KB

Okay after some more review I think I realize what I was doing wrong. You really don't want to just check !empty but probably use is_array instead - to ensure the $menu['type'] is something that can be iterated over.

tim.plunkett’s picture

FileSize
22.57 KB

Crosspost of sorts, I started this after I saw #30. We'll see how it fares in comparison.

The last submitted patch, 32: vdc-2027043-32.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 33: vdc-2027043-33.patch, failed testing.

saltednut’s picture

Status: Needs work » Needs review
FileSize
22.7 KB

Default config has moved into config/install

Also, we missed a view: core/modules/user/config/install/views.view.user_admin_people.yml

saltednut’s picture

FileSize
497 bytes
22.7 KB

Of course I'd post a patch with whitespace errors... >:)

saltednut’s picture

Status: Needs review » Needs work

Passing with testbot now but still needs test coverage per #17

mgifford’s picture

Issue tags: +Needs reroll

As said above, needs "some kind of integration test".

Status: Needs work » Needs review

mgifford queued 37: vdc-2027043-37.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 37: vdc-2027043-37.patch, failed testing.

Désiré’s picture

Assigned: Unassigned » Désiré
Désiré’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll +Amsterdam2014
FileSize
23.27 KB

Rerolled the patch. Seems to work.
Now I'll make some tests.

Status: Needs review » Needs work

The last submitted patch, 43: vdc-2027043-43.patch, failed testing.

jibran’s picture

Status: Needs work » Needs review
FileSize
23.26 KB

Here is the reroll more later.

Status: Needs review » Needs work

The last submitted patch, 45: vdc-2027043-45.patch, failed testing.

jibran’s picture

Status: Needs work » Needs review
FileSize
1.8 KB
24.62 KB

Next step make it green.

Status: Needs review » Needs work

The last submitted patch, 47: vdc-2027043-47.patch, failed testing.

jibran’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
1.88 KB
25.41 KB

With one assertion.

jibran’s picture

Some minors.

  1. +++ b/core/modules/views/src/Plugin/views/display/Page.php
    @@ -358,12 +369,14 @@ public function validateOptionsForm(&$form, FormStateInterface $form_state) {
    +      form_set_value($form['menu']['type'], $menu_types, $form_state);
    

    deprecated.

  2. +++ b/core/modules/views/src/Plugin/views/display/PathPluginBase.php
    @@ -305,33 +305,40 @@ public function getMenuLinks() {
    +          // Some views might override existing paths, so we have to set the route
    

    more then 80 chars.

jibran’s picture

FileSize
3.97 KB
26.25 KB

Fixed #50 and added some more asserts.

dawehner’s picture

Assigned: Désiré » Unassigned

.

jibran’s picture

FileSize
904 bytes
26.24 KB

blah :/

dawehner’s picture

+++ b/core/modules/node/tests/modules/node_test_views/test_views/views.view.test_contextual_links.yml
@@ -79,7 +79,8 @@ display:
+        type:
+          tab: tab

diff --git a/core/modules/user/config/install/views.view.user_admin_people.yml b/core/modules/user/config/install/views.view.user_admin_people.yml
index 854d128..f280fef 100644

+++ b/core/modules/user/config/install/views.view.user_admin_people.yml
@@ -877,7 +877,8 @@ display:
+        type:
+          'default tab': 'default tab'
         title: List

Just 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

vijaycs85’s picture

#54 ideally we want to have it like:

type:
  - tab

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.

dawehner’s picture

Category: Feature request » Task

This 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?

vijaycs85’s picture

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?

Sure, I remember we fixed this in few places initially (when converted variable_get to config).

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Okay, cool

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 53: vdc-2027043-52.patch, failed testing.

Status: Needs work » Needs review

dawehner queued 53: vdc-2027043-52.patch for re-testing.

olli’s picture

Manually tested the patch trying to add files view to toolbar and it works!! Two questions:

  1. +++ b/core/modules/views/src/Plugin/Derivative/ViewsLocalTask.php
    @@ -108,29 +110,31 @@ public function alterLocalTasks(&$local_tasks) {
    +      foreach ($menu['type'] as $type) {
    +        if (in_array($type, array('tab'))) {
    

    Isn't this same as if (in_array('tab', $menu['type']))?

  2. +++ b/core/modules/views/src/Plugin/views/display/Page.php
    @@ -159,14 +164,20 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
    -          '#type' => 'radios',
    +          '#type' => 'checkboxes',
    +          '#multiple' => TRUE,
               '#options' => array(
                 'none' => $this->t('No menu entry'),
                 'normal' => $this->t('Normal menu entry'),
    

    Can we drop the 'No menu entry' option now?

blackra’s picture

Status: Needs review » Needs work

I 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.

olli’s picture

Oh, I see, #61.1 is not same because for now the menu[type] is not filtered.

  1. +++ b/core/modules/views/src/Plugin/views/display/Page.php
    @@ -358,12 +369,14 @@ public function validateOptionsForm(&$form, FormStateInterface $form_state) {
    +      $menu_types = $form_state->getValue(array('menu', 'type'));
    +      $form_state->setValueForElement($form['menu']['type'], $menu_types);
    +
    +      if (in_array('normal', $menu_types) && strpos($path, '%') !== FALSE) {
             $form_state->setError($form['menu']['type'], $this->t('Views cannot create normal menu items for paths with a % in them.'));
           }
     
    -      if ($menu_type == 'default tab' || $menu_type == 'tab') {
    +      if (in_array('default tab', $menu_types) || in_array('tab', $menu_types)) {
             $bits = explode('/', $path);
             $last = array_pop($bits);
             if ($last == '%') {
    @@ -371,7 +384,7 @@ public function validateOptionsForm(&$form, FormStateInterface $form_state) {
    
    @@ -371,7 +384,7 @@ public function validateOptionsForm(&$form, FormStateInterface $form_state) {
             }
           }
     
    -      if ($menu_type != 'none' && $form_state->isValueEmpty(array('menu', 'title'))) {
    +      if (!in_array('none', $menu_types) &&  $form_state->isValueEmpty(array('menu', 'title'))) {
    

    Similarly here the $menu_types is not filtered so these in_array()s are always true, right?

  2. +++ b/core/modules/views/src/Plugin/views/display/Page.php
    @@ -406,13 +419,13 @@ public function validate() {
    -      if (!empty($tab_options['type']) && $tab_options['type'] != 'none' && empty($tab_options['title'])) {
    +      if (!empty($tab_options['type']) && empty($menu['type']['none']) && empty($tab_options['title'])) {
             $errors[] = $this->t('Display @display is set to use a parent menu but the parent menu link text is not set.', array('@display' => $this->display['display_title']));
           }
    

    That empty() in the middle might be the cause for the second issue in #62.

selecting the 'default menu tab' option caused the parent link option to disappear

That sounds like there is some #states conditions that should be updated.

jibran’s picture

Status: Needs work » Needs review
FileSize
6.88 KB
46.29 KB

Here is a reroll and fixes for #61 and #63. Thanks @olli awesome observation.

jibran’s picture

  1. +++ b/core/modules/node/config/install/views.view.content.yml
    @@ -1,3 +1,4 @@
    +uuid: 3726fb5a-ed6c-4c0f-b20e-14f6108dd984
    

    Sorry about that :(. I hope it'll not fail because of this.

  2. +++ b/core/modules/node/config/install/views.view.content.yml
    @@ -372,8 +357,6 @@ display:
    -          entity_type: node
    -          entity_field: type
    

    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.

Status: Needs review » Needs work

The last submitted patch, 64: 2027043-64.patch, failed testing.

jibran’s picture

Status: Needs work » Needs review
FileSize
22.72 KB
28.82 KB

With the test fix.

Status: Needs review » Needs work

The last submitted patch, 67: 2027043-67.patch, failed testing.

jibran’s picture

Status: Needs work » Needs review
FileSize
1.17 KB
28.84 KB

:/

Status: Needs review » Needs work

The last submitted patch, 69: 2027043-69.patch, failed testing.

dawehner’s picture

@jibran
Is the last one on purpose?

jibran’s picture

No 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?

olli’s picture

Status: Needs work » Needs review
FileSize
34.28 KB
5.7 KB

This fixes the test. Adjusted #states also.

olli’s picture

FileSize
1.97 KB
32.58 KB
olli’s picture

FileSize
32.46 KB
831 bytes
+++ b/core/modules/views/src/Plugin/views/display/Page.php
@@ -437,13 +449,13 @@ public function validate() {
-      if (!empty($tab_options['type']) && $tab_options['type'] != 'none' && empty($tab_options['title'])) {
+      if (!empty($tab_options['type']) && empty($menu['type']['none']) && empty($tab_options['title'])) {
         $errors[] = $this->t('Display @display is set to use a parent menu but the parent menu link text is not set.', array('@display' => $this->display['display_title']));

tab_options type can still be 'none'.

jibran’s picture

Awesome @olli thanks for fixing this.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/core/modules/views/src/Plugin/views/display/Page.php
@@ -91,7 +91,8 @@ protected function defineOptions() {
+        // Do not translate menu and title as menu system will.

No, config actually will, but let's just drop the comment, its not that helpful.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs issue summary update

This 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.

dawehner’s picture

Issue summary: View changes

Added ISBE.

jibran’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs issue summary update

Back to RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 75: 2027043-75.patch, failed testing.

jibran queued 75: 2027043-75.patch for re-testing.

jibran’s picture

Status: Needs work » Reviewed & tested by the community

Back to RTBC.

xjm’s picture

TBH 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".

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/views/src/Plugin/Derivative/ViewsLocalTask.php
@@ -108,29 +110,31 @@ public function alterLocalTasks(&$local_tasks) {
+      foreach ($menu['type'] as $type) {
+        if (in_array('tab', array_filter($menu['type']))) {

This 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?

olli’s picture

Status: Needs work » Needs review
FileSize
29.98 KB
2.66 KB

Fixed #85 by removing that foreach.

jibran’s picture

@dawehner should we move it to D9 then?

xjm’s picture

Version: 8.0.x-dev » 9.x-dev
Category: Task » Feature request
Status: Needs review » Postponed

Yeah, 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.

jibran’s picture

Well 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.

dawehner’s picture

Version: 9.x-dev » 8.0.x-dev
Category: Feature request » Bug report
Status: Postponed » Needs review

Yeah, 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.

Alright, 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.

dawehner’s picture

This time with a screenshot

dawehner’s picture

Issue tags: +Usability

Fixing this would certainly improve usablity

dawehner’s picture

+++ b/core/modules/views/src/Plugin/Derivative/ViewsLocalTask.php
@@ -69,25 +69,27 @@ public function getDerivativeDefinitions($base_plugin_definition) {
       $menu = $executable->display_handler->getOption('menu');
-      if (in_array($menu['type'], array('tab', 'default tab'))) {
-        $plugin_id = 'view.' . $executable->storage->id() . '.' . $display_id;
...
+      foreach ($menu['type'] as $type) {

What 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 ...

jibran’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll
FileSize
22.42 KB

Here is a screenshot after enabling admin_toolbar.

olli’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
30.07 KB

Reroll

dawehner’s picture

Issue summary: View changes

Status: Needs review » Needs work

The last submitted patch, 95: 2027043-95.patch, failed testing.

olli’s picture

Status: Needs work » Needs review
FileSize
30.68 KB
622 bytes
dawehner’s picture

Version: 8.0.x-dev » 8.1.x-dev
Category: Bug report » Task

Alright, 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?

jibran’s picture

slashrsm’s picture

Issue tags: +sprint
slashrsm’s picture

Issue tags: +D8Media
Berdir’s picture

It'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 :)

Crell’s picture

FYI, 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.

dawehner’s picture

Category: Task » Bug report

IMHO, this is a bug :)

I 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.

jibran’s picture

Bug for 8.1.x doesn't make sense to me.

Crell’s picture

Version: 8.1.x-dev » 8.0.x-dev

I concur. Especially if right now the alternative leads to a fatal site. :-)

dawehner’s picture

@Crell
Oh wait, does that issue fixes the other issue?

olli’s picture

FileSize
30.67 KB

rebase

Status: Needs review » Needs work

The last submitted patch, 109: 2027043-109.patch, failed testing.

Crell’s picture

dawehner: 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.

xjm’s picture

Version: 8.0.x-dev » 8.2.x-dev
Issue tags: -beta target

This 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.

Gábor Hojtsy’s picture

  1. +++ b/core/modules/file/config/optional/views.view.files.yml
    @@ -725,12 +725,14 @@ display:
           menu:
    -        type: tab
    +        type:
    +          tab: tab
    

    Should this now use the new capability and define it as a menu item as well? Found while reviewing with @tamas.gergocs

  2. +++ b/core/modules/views/config/schema/views.display.schema.yml
    @@ -23,8 +23,11 @@ views.display.page:
             type:
    -          type: string
    +          type: sequence
               label: 'Type'
    +          sequence:
    +            - type: string
    +              label: 'Menu type'
    

    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.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Gábor Hojtsy’s picture

Issue tags: -Media Initiative

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

vijaycs85’s picture

Priority: Normal » Major
Issue tags: -D8Media +Media Initiative

There 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?

dawehner’s picture

Issue tags: +Needs reroll
jofitz’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
30.2 KB

Re-rolled.

Status: Needs review » Needs work

The last submitted patch, 120: 2027043-120.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

jofitz’s picture

Status: Needs work » Needs review
FileSize
3.94 KB
34.14 KB

Address some of the test failures.

Status: Needs review » Needs work

The last submitted patch, 122: 2027043-122.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

jofitz’s picture

Status: Needs work » Needs review
FileSize
830 bytes
34.13 KB

Correct error in re-roll.

Status: Needs review » Needs work

The last submitted patch, 124: 2027043-124.patch, failed testing. View results

jofitz’s picture

Status: Needs work » Needs review
FileSize
2.2 KB
34.26 KB

Found and corrected a couple more re-roll errors and fixed 2 coding standards errors.

Status: Needs review » Needs work

The last submitted patch, 126: 2027043-126.patch, failed testing. View results

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

tea.time’s picture

It'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...

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

nikitagupta’s picture

Status: Needs work » Needs review
FileSize
33.46 KB

Reroll patch #126.

nikitagupta’s picture

worked on test case failures and deprecation.

andypost’s picture

Tests failed

Drupal\Tests\hal\Functional\Update\HalSettingsDeletionUpdateTest::testUpdate
Exception: Warning: foreach() argument must be of type array|object, string given
Drupal\views\Plugin\views\display\PathPluginBase->getMenuLinks()() (Line: 350)

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

SocialNicheGuru’s picture

i 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.

catch’s picture

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.