contact_menu_local_tasks_alter() should check whether $data['tabs'][0] is set

If there is a bug exposed by this, we should presumably have tests? Thanks!

and yep that needs tests

+++ b/core/modules/contact/contact.module
@@ -99,7 +99,7 @@ function contact_entity_extra_field_info() {
+  if (($route_name == 'entity.user.canonical') && isset($data['tabs'][0])) {

probably Element:children($data[tabs])

I would like to add a test, but I am not sure what to test for. Can someone point me in the right direction?

I found this test in contact/src/Tests/ContactPersonalTest.php:

    // Test that the 'contact tab' does not appear on the user profiles
    // for users without an email address configured.
    $this->drupalGet('user/' . $this->contactUser->id());
    $contact_link = '/user/' . $this->contactUser->id() . '/contact';
    $this->assertResponse(200);
    $this->assertNoLinkByHref ($contact_link, 'The "contact" tab is hidden on profiles for users with no email address');

I think the test would have to unset the tabs, then visit the user contact page and make sure it loads OK (200).

so, when @andypost mentions Element:children($data[tabs])
maybe that is a hint that this doesn't have to be a simpletest with a drupalsite, and can be a phpunit test on the output of the method?

If you look at menu_local_tasks() there is the chance that $data['tabs'][0] is empty indeed.

I think the test would have to unset the tabs, then visit the user contact page and make sure it loads OK (200).

I mean it is really hard to trigger because /user/{user} is either accessible, so is the tab, or we result in a 403,
in which case $route_name is 'system.403', so I doubt you can ever run into that case in real life.

I hit this bug due to another bug, where Drupal was trying to render the local tasks on a maintenance page in which case there are no local tasks (will open a separate bug for that).

Do you mind pasting an URL to that? Why do we even try to produce local tasks on the maintenance page.

Wrote phpunit test which tests for the undefined index. This is the opposite of what we actually want the test to do but could not figure how to test for a missing undefined index notice.

@tatisilva this test should be in .patch format

I have changed the format from .txt to .patch. Hope, this will solve the issue.

Looks good start!

+++ b/core/modules/contact/tests/src/Unit/ContactTest.php
@@ -0,0 +1,37 @@
+class ContactTest extends UnitTestCase {

I bet you need to add contact module to the list of installed ones

Hope, this patch would fix the issue.

currently i am working on it ....

Please review it..... i have updated the patch .....

Please find the updated patch ..

We've now got a test only file, but no fix.

And the test-only file doesn't fail.

Can someone confirm if this issue still exists?

Add isset checking for tabs and corresponding test cases in MR 346. Please review the changes.

I think we're probably better to remove that test and try to test it using the maintenance page approach per the issue summary steps to reproduce

Yeap, the test failed now because it is expecting the broken behaviour and we've fixed it, but it needs to be the other way around

Test file removed. Updated changes available in MR346. Please review.

Seems to be a simple isset() check. I see the test file was added and then removed so not sure if a new one is needed?

Also may need a reroll for 9.5.x. The MR is aimed at 9.2.x

But following the testing steps

Make sure contact module is installed
Turn maintenance mode on
Went to /user/1 because the code checks $route_name == 'entity.user.canonical'

Not sure how to trigger local tasks but there are no local tasks.

Would say this needs an issue summary update (so move to PNMI)
Or maybe closed as can't reproduce.

Btw #34 ist the best test to reproduce (

Updated the patch #34. Uploading a tests only patch.

1. +++ b/core/modules/contact/tests/src/Unit/ContactTest.php
   @@ -0,0 +1,36 @@
   +use EasyRdf\Exception;
   

   This is the wrong namespace, I think we just want to use \Exception in the code (ie with the \), however read on below, I think we don't need either.

2. +++ b/core/modules/contact/tests/src/Unit/ContactTest.php
   @@ -0,0 +1,36 @@
   +  /**
   +   * Modules to enable.
   +   *
   +   * @var array
   +   */
   +  protected static $modules = ['contact'];
   

   I don't think this is supported/required with a UnitTestCase, so we can remove it

3. +++ b/core/modules/contact/tests/src/Unit/ContactTest.php
   @@ -0,0 +1,36 @@
   +    $route_name = 'entity.user.canonical';
   

   I don't think we need a local variable for the route name, lets just pass it straight to the function

4. +++ b/core/modules/contact/tests/src/Unit/ContactTest.php
   @@ -0,0 +1,36 @@
   +    try {
   ...
   +    }
   +    catch (Exception $e) {
   +      $this->fail('Exception thrown');
   +    }
   

   PHPUnit will fail if there's an unhandled exception, so we don't need the try catch

5. +++ b/core/modules/contact/tests/src/Unit/ContactTest.php
   @@ -0,0 +1,36 @@
   +      $this->assertTrue(TRUE, 'No warning thrown');
   

   Instead of this, I think we test the valid use-case (ie when $data['tabs'][0] is set), so then we have a positive assertion that expands coverage of that method rather than testing for just the exception case - thoughts?

Try to Addressed Comment #52
Still need to work on Point 5.

So contact_menu_local_tasks_alter doesn't return anything so how would be best to test?

I assume it alters by reference (on phone didn't check)

The next task here is to convert the latest patch to an MR, that is suitable for a first issue.

Fixed pipeline issues, please review it.

Thanks!

Tweaked the test slightly but test-only shows the test coverage and code fix is straight forward.

I read the issue, comments and the MR etc. I found all questions answered and all core gates have been addressed. I think this is good to go.

I have updated credit.

Assigning to myself to commit within 24 hours.

Thanks!

Committed to 11.x, 11.0.x, 10.4.x, and 10.3.x.