Views does not create parent local task for a default local task

Working on this in the mentored core sprints in Dublin.
First step: Manual test (https://www.drupal.org/contributor-tasks/manual-testing)

I m working on this issue
dublin mentored sprints
i ll manually review the patch https://www.drupal.org/contributor-tasks/manual-testing

I am working on the issue at the mentored Sprints @ DC Dublin, doing manual review as suggested here: https://www.drupal.org/contributor-tasks/manual-testing

Patch applies cleanly against latest 8.3.x dev
Problem reproduced and problem was solved

+1 for applies cleanly and confirming that the bug is there and the patch does solve it.

I followed the steps and reproduced the issue. The patch applies cleanly and solves the problem

Switching back to 'Needs Review', we are going to test the coding standards

This patch does not introduce any coding standards issues.
There are however some coding standards issues on the code indentified before applying the patch. Here is the relevant issue: https://www.drupal.org/node/2809251#comment-11677139

Some code refactoring for:

a) Code readability: Using a variable for self-documenting the kind/role of the argument passed to the applyBaseRoute function.
b) Possible code smell: hard coding a string concatenation inside a function call, in two different places, seemed a bit problematic. Tried to make this a bit more robust.
c) Extending the applyBaseRoute function to cover two use cases by adding an extra argument to the parameters list of the function.

Providing an interdiff as well as a re-rolled patch.

I think one thing we should try here is some kind of integration test coverage. Having a real world example would add quite some value.
Note: The issue is also tagged as "needs manual testing", so we need to address that as well.

I manually tested this patch and it works as expected.

Manually tested the latest patch file as follows:

Set up a clean install of D8.3.x on simplytest.me
Followed steps to reproduce
Confirmed that "Parent" tab is not displayed on admin/content

Set up a D8.3.x install with the patch applied on simplytest.me
Followed steps to reproduce
Confirmed that "Parent" tab is correctly displayed on admin/content

Patch no longer applies to 8.3.x or 8.4.x.

Patch re-rolled for 8.x-4.x and confirmed that it is working.

So we still need an integration test per #15.
Then AFAICS we could remove the "Needs manual testing" tag.
It would also be good to have a separate test-only patch.

Removed "Needs reroll" tag.

Corrected coding standard errors - namely short array syntax.

Setting back to Needs Work for the test mentioned in #15.

I think enough work has been done on this issue that it is time for an update to the summary. Whoever does that should decide whether the Novice tag is still appropriate.

+++ b/core/modules/views/src/Plugin/Derivative/ViewsLocalTask.php
@@ -127,26 +146,51 @@ public function alterLocalTasks(&$local_tasks) {
+  protected function applyBaseRoute(ViewExecutable $view, $plugin_id, &$local_tasks, $is_parent = FALSE) {

this would make this an API change, any way to keep this inline to prevent overlapping method names with anything extending this somewhere?

And yeah, ++ to adding some functional tests

Here is a way we could go to avoid adding a new method (in order to avoid API changes, see #30).

Here is a functional test by means of expanding DisplayPageWebTest::testPageDisplayMenu.

I'm unable to create a menu tab when the URL is dynamic.

In the summary example, if you switch the URL to node/%node/test/one (and add a contextual filter), you would expect to see the Parent tab on a node page but it's not happening.

Cache clear made it work.

Sitting with jhodgdon at PNW Drupal Summit and we reviewed the functional test. It looks like it's testing the bug. We weren't sure about the unit test and we didn't review the code patch.

We made a test-only patch to verify that the test fails without the patch.

We looked at the test results and noticed there was a coding standard problem with the patch on #34. So here's a new patch.

Going on a limb here and setting this to RTBC, we have reviewed tests now, I believe that's all that was still missing here?

#2027043: Allow page displays to be both local tasks and menu items is related possible duplicate.

I confirm that the problem was reproduced and patch works properly . I inserted small changes according used standards.

@dawehner asked for integration tests in #15 and I still think this patch could use it - i.e. create the view and check the actual tabs show up.

@catch I thought I added that on #34 - is this not what you mean?

Indeed, that looks like exactly what that test is testing for. ;) And I confirmed the test survived subsequent re-rolls and is still present in #42.

Tentatively back to RTBC... (although, disclaimer: I haven't actually closely reviewed this, ran tests locally, manually tested, etc).

I came across this problem when creating a view with two displays on 2 tabs, one of which is "Default menu tab" and "Normal menu item". The parent item was not created.
The paths were /foo/bar and /foo/baz so I'd expect the parent to be /foo.
The view also takes a nid parameter after the path so the "real" paths are /foo/bar/{arg_0} and /foo/baz/{arg_0}.
The code that constructs the parent URL was adding the "missing arguments" before constructing the parent path by trimming off the last bit of the path, so it ended up with /foo/bar and /foo/baz.
My fix is to prevent it adding "missing arguments" when constructing a parent path. Patch attached (generated from PhpStorm local history).

Thank you @dww for the manual testing.

Also thank you @hughworm for sharing your solution. However there is already a patch that a lot of people have participated in creating, and that includes the test coverage for the bug described in the issue summary.

A better way of helping would be testing if the latest patch solves your problem as well, reviewing it in comparison to what you did, and giving your opinion on it.

Setting it back to RTBC, correct patch is #42.

Re-uploading #42 for clarity.

+++ b/core/modules/views/src/Plugin/Derivative/ViewsLocalTask.php
@@ -71,7 +71,7 @@ public function getDerivativeDefinitions($base_plugin_definition) {
-      /** @var $executable \Drupal\views\ViewExecutable */
+      /* @var $executable \Drupal\views\ViewExecutable */

@@ -110,15 +128,17 @@ public function alterLocalTasks(&$local_tasks) {
-      /** @var $executable \Drupal\views\ViewExecutable */
+      /* @var $executable \Drupal\views\ViewExecutable */

Nit: Non related change, but in any case, core is standardizing on this format instead: /** @var \Drupal\views\ViewExecutable $executable */ (see #2305593: [policy] Set a standard for @var inline variable type declarations).

Apologies @manuel-garcia I should have been clearer.

I had tried patch #42 without success. After applying that patch (and clearing caches) pages /foo/bar and /foo/baz loaded but with no menu tab, and the parent menu /foo was not created (and gave 404).

As I described in #46 I think having an hidden argument in each path (uid defaulted from the current user) causes the problem.

But after my additional patch at #46 the pages /foo and /foo/baz loaded as tabs, but ONLY after I manually added the /foo menu item. (/foo/bar gives 404, which I guess is correct).

@manuel-garcia, thanks for clarification. If we don't need in changes with accordance your comment #51, i think that we can return to patch in #38. In this case my comment can be reviewed as confirmation that bug was and it fixed by patch in #38.
@hughworm, i confirmed in my comments that bug was reproduced and patch in #38 its fixed. I checked it on fresh installation of D 8.6 and with reproducing of steps which were describe in issue. Possible that this patch doesn't cover all situations as in your case for example.

Good point @andrew-as - re-uploading #38 here.

@andrew-as and @manuel-garcia I tried again with just patch #38 applied and got the same result. I had to apply my patch from #42 to get it to work. Would you like me to roll a combined patch?

Maybe, make sense to create new child issue. @hughworm, you can place your patch here. And you could put more details into part of reproduce steps in its description. Is it more preferred? What do you think?

Sorry for taking so long to have a look at this. The issue summary still needs an update and it also would be great to a little bit more sub-system maintainer review.

The other thought that occurs to me is what about existing sites that have worked around the problem by creating a parent themselves via code or something? I.e. I think we need to think about backwards-compatibility and upgrade path. Even if the upgrade path is just to ensure that the views local task derivatives are re-generated.

@jonathanshaw I'm not sure we need an upgrade path - I think we just need to consider it.

Re #57

The other thought that occurs to me is what about existing sites that have worked around the problem by creating a parent themselves via code or something? I.e. I think we need to think about backwards-compatibility and upgrade path. Even if the upgrade path is just to ensure that the views local task derivatives are re-generated.

That is a good point... what would happen if there is already an existing parent local task and we apply this patch? If something breaks then we do need an upgrade path, otherwise I think we could just inform in the CR that existing sites working around the bug should consider removing the workarounds.

So we should manually test this scenario, to be on the safe side.

I'm also a bit uneasy about the level of test coverage. While I see the point of the unit test, I think it mostly proves that the code behaves as it behaves :)

One amazing thing would be to list all the possible cases in the issue summary. We could then check whether all cases are covered by tests easily.

Thanks for the good work here, creating tabs from views is a really handy feature.
I tried only using patch from #54 on my 8.6.1 site but I'm unable to let views create the parent link in the menu of my choice,...
So I'm also using the workaround for now by creating a manual link using the menu interface of update hook

Hi,
Patch #54 applied just fine on an 8.9.1 installation.
However, although the parent tab is correctly added, the child route doesn't seem to be created.

After applying the patch, clearing the cache, and following the "Steps to reproduce" in the issue summary, the admin/content/test URI works as expected, but admin/content/test/one returns a 404 page.
Since admin/content/test/one returns 404, a display created with the URI admin/content/test/two with a tab link will not have any tabs at all.

Here are the full steps to reproduce my scenario (the steps in bold are the added ones):

1. Apply patch
2. Clear cache
3. Edit the Content view (admin/structure/views/view/content/edit)
4. Add a new page display
5. Use path admin/content/test/one
6. Add a menu item entry with Type "Default tab" and Menu link title "Child"
7. For the Parent menu item, choose "Menu tab" with Menu link title "Parent"
8. Add a new page display
9. Use path admin/content/test/two
10. Add a menu item entry with Type "Tab" and Menu link title "Second Child"
11. Save the View
12. Visit admin/content expecting to see tab "Parent" appear. Scratch your head and curse the heavens because it's not there.and note that the tab "Parent" does exist.
13. Visit admin/content/test and note that the route does exist. It's just the local tasks that haven't been created properly.
14. Visit admin/content/test/one expecting to see the same page as admin/content/test/, but get a 404.
15. Visit admin/content/test/two and note that the route does exist, but no tabs appear.

Looking at comment #52, it might be intentional that admin/content/test/one returns 404, but I would argue that it shouldn't, otherwise I don't see the point of setting a URI here.

I think, that this is right behavior, when default tab url don't show duplicated page of parent url, but with different url. This is good for prevent showing same info on two different urls.

But showing 404 error is not so good response for this. Better way is making 301 redirect to parent url from default tab url, is it hard to implement?

And problem 15 from #67 is not reproduced on my setup, I see both tabs on page of "two" tab.

Sounds like this Needs work based on #67, I also don't think we need a child issue to fix @hughworm's issues from #46. To me that sounds like a bug with this issue.

I've closed #2958746: Views does not create local tasks that have a missing argument in favor of fixing that bug here as well.

Rerolled patch against 9.4.

Rerolled patch for #74 against 9.4.
Fixed custom commands.

patch in#77 applies successfully and working fine for me on local 9.5.

I have successfully applied patch #77 on drupal 9.5.x-dev and its working for me
Thanks @ranjith_kumar_k_u. Adding screenshots for the refrence

Reviewing RTBC issues to check they pass the core gates.

@gayatri chahar, thanks for the interest in this issue. However, a working patch is not sufficient to set the status to RTBC. I am not sure why you have added a second set of screenshots for the same patch. Ah, I see they are for a different theme. When adding screenshots, it is good practice to explain what you did. Also, screenshots should be placed in the Issue Summary to help everyone working on the issue.

This issue still needs: Needs issue summary update, Needs subsystem maintainer review, Needs upgrade path.

Setting back to Needs work for those to be addressed.

Reroll the patch #77 with Drupal 10.0.x

patch #81 applies successfully and working fine for me on on drupal 10 and 10.1

For me patch #81 failed to apply on 10.0.x-dev

D10 version needed
At this time we need a D10.1.x patch or MR for this issue.

And per #80 This issue still needs: Needs issue summary update, Needs subsystem maintainer review, Needs upgrade path.

Hi there!
Patch #81 is accepted as an error for me, but I took it as a base and tweaked it a bit after looking at comment #67.
So the attached patch contains changes from patch #81 and temporarily implemented logic for setting up local tabs.
The patch contains improvements in line with the new features provided by PHP 8
This patch does not contain tests, and needs to be written and tested, or improved.
Patch Tested on Drupal version 9.5.0 and 10.0

Steps for testing that I carried out:
1. Create a view with the path /user/test/default/tab_1
2. Add a menu item with type "TITLE TAB" and link name "DEFAULT MENU TAB 1"
3. Create a new display view, or another view
4. Add a menu item Menu tab with the path /user/test/default/tab_2 and the name "DEFAULT MENU TAB 2"
5. Save view and clear cache
6. Go to the user's page and see the result as in the picture

P.S. Any advice/criticism on the comment would be appreciated

Hi all again, looking through this patch I found that when adding local tasks to an existing route, they don't show up, so I updated the patch for that, also added the tests from patch #81

However, I still believe that this patch needs better tests, as well as code verification, maybe there is a better option to solve this problem

Fixed CCF for #87.
Please review.

Found that the block disappeared using #88 patch with D9.5.x.
Updated the patch file to restore it.

Took patch 88 and updated it to Drupal 10.3

I put the patch from #92 into an MR, and also incorporated the change introduced in #91, since I did observe that the tabs were missing, and that fixed it for me.

All that being said, I don't see the expected menu item after applying these changes, so leaving the issue as Needs work.

UPDATE: I realized, in working on this more, that I was actually trying to solve a different use case: have the default menu tab provide a normal menu item. As such, I wasn't really testing the right things. I'll continue fixing my own problem in #3467262: For a page display designated "Default menu tab" a parent menu link is not created.

That said, there's still some work needed to get the unit tests passing here. I was able to fix the linters, but it looks like a little more work is needed to get the PHPUnit checks passing.

Rebased the MR.

Attached patch is a snapshot of the latest state of the MR for safe usage with composer patches.

Fixed PHPSTAN and PHPCS. The unit test is failing.

Attached patch is a snapshot of the latest state of the MR for safe usage with composer patches.

Just want to reiterate what was said in #15, having an integration level test would be really great here. Just a preconfigured View with tabs and just check they show up, something along those lines.