Help link always appears in navigation

I attempted to reproduce this issue by installing Drupal 11 with the Minimal profile, but I was unable to reproduce the exact behavior described in the issue. Here's what I observed:

The help link in the navigation module appears only if:
-The Help module is installed and placed in the left sidebar region.
-The cache is cleared afterward to ensure the link becomes visible.
-When the Help module is uninstalled, the link disappears as expected.

Based on these observations, I could not confirm the issue as reported. However, I identified a potential enhancement to address this scenario if it arises. I have added the following temporary change to the navigation.block_layout.yml file:

additional: { }
dependencies:
module:
- help

This change ensures that the Help module dependency is explicitly defined, potentially resolving any unexpected behavior related to the navigation link.

Hi
I have tested MR !10363 on Drupal version 11.x with minimal Profile
MR was applied successfully,
but Help still appears in Navigation instead of the module that was not installed, and the Help Page displays a 404 error.

Attaching screenshots

moving to "Needs work"

Tested locally the fix 35b58e49 - Fix: Prevent Help link from being displayed when Help module is not enabled.
It works for the version 11.Xdev.
Changing status to needs review.

Thank you for jumping into this!

Some comments added in the MR that might need to be addressed.

Work done on the MR to resolve comments. Fixed PHPCS and other errors. Removed duplicate try catch blocks.

Test coverage still needed.

I am not sure that the new access checking code is needed. Seems like this could be a one-liner fix. Just check whether the help module is enabled. Possibly create a fresh branch. Not sure that adding help module as a dependency in the .yaml is necessary..

Further manual testing is required.

Thank you for stepping up into this @oily.

The NavigationLinkBlock can support any link. In this very specific case is a link to the help page, but it could be a link to anywhere. That's the reason why we cannot add any one-liner just to check whether the help module exists, because the bug would be present for other links. We need a generic way to solve it.

Agree that references to help in the yml file are not needed.

Would be amazing to include basic automated tests for this as well. They'll be necessary to approve the MR.

Hi @plopesc, I have noticed that the MR varies a lot from the current Drupal 11.x versions of the same 2x files.

I suggest we create a new branch from 11.x and hide the existing branch. The code in it may have made sense for Drupal 8 or 9 but it seems just a source of confusion now. I think it best to start from a clean slate.

We need to confirm that this issue has not been fixed already in Drupal 11.x. Up-to-date screenshots would help.

If it is still a valid issue, we need to work out how and where and in which file to implement the logic that checks whether the help module is enabled and if it is not to not display the link in the navigation block.

I agree that the navigation block contains a dynamic set of links and they will involve other modules than the help module. So The whole block should not be dependent on the help module. But the link to the help module should be.

Created fresh branch forked from 11.x. Hid the old branch.

Thank you @oily.

Even if the old branch was not actually outdated, agree that we had some noise there. On the other hand, access to the history of the discussions is a bit more complex.

Took the generic bits from the original MR and added test coverage. This should be ready for review now.

Could you please remove the "Draft" state from the MR? It seems I don't have permissions for that since you created it.

Hi
I have applied MR! 10662 on Drupal 11.x with a minimal Profile
The MR is applied successfully...

Test Result: PASS
If the Help module is not installed, the Help link will not appear in the navigation menu.
If the Help module is installed, the Help link will appear in the navigation menu and redirect to the appropriate page.

Attaching Screenshot

RTBC+1

But MR is in Draft state so keeping the issue in "needs review"

Removed draft status of the new 11.x branch.

@plopesc Thank you for the code. Great that you created a test, too! I confused this with another issue that dates back to Drupal 8. We seem to be making progress on the new branch.

@kanchan bhogade Thank you for the review. Looks good! Not sure if you have run the 'Test-only' test? That should fail. It can only be run manually. You have to press the start button beside it. If that fails then it indicates that the test coverage is working as it should.

The 'test-only' test fails as it should. Changing to RTBTC.

Hi @oily, noticed that you reverted back the issue from RTBC to Needs Review with no comment.

Would be great if you could explain the reasons behind that change, so we would be able to work on them.

Thank you in advance.

@plopesc I am not sure about this but I have noticed several of tickets I am following set to RTBTC which have not automatically fixed within 2 weeks as I thought they should. So I moved 2 such tickets back to NR as I thought perhaps if a core maintainer reviews and sets to RTBTC then it will auto-fix in 2 weeks. I also thought that perhaps NR would get the attention of a maintainer whereas RTBTC might be a status that will be ignored for a longer period.

Also, yesterday I was taken to task by a maintainer for setting an issue to RTBTC without doing a technical review of the code. In this case I am not sure I am the right person to do that in this case but a maintainer certainly would be.

I think everyone involved in this issue wants it to be set to 'fixed' asap. But setting it to RTBTC will not get it fixed within 2 weeks and could mean an indefinite wait.

@oily I want to encourage you to review the documentation on the issue Status. It might clear up some confusion.

For this issue, and in general, you'd ideally have someone who did not contribute to the code to do a technical review and testing. That person, given they found no issues, could then move the issue to RTBC. RTBC issues remain in that state until a core committor has time for a final review before commiting. Once committed, it would move to Fixed. Then if there are no further comments or activity, it woul automatically move to Closed (Fixed). Again, review the issue Status docs for more details.

I'll go through the MR to review and test. It looks like there is a test failure to handle. I'll see if I can work out why that is.

Test failures we're fixed by merging the latest 11.x.

Bug fix works as advertised and the test is super comprehensive. Nice work on that @plopesc!

Marking as RTBC.

@m4olivei WRT #32 I had a core maintainer quibble with me over these matters when another person reviewed an MR on another issue. I really dont wish to review all these documents/ issues right now. I am pleased you have stepped in to progress this issue. Hopefully a maintainer will step in soon. But that maintainer might not hurry if they do not see something more than 'code looks great!' but rather at least one line of feedback on the code, some small improvement. It is not an exact science. In this issue we have a coder with more experience and a reviewer with more experience than in the other issue so all-in-all more likely the code IS 100% good. Such matters as experience level is not factored into the documents but maybe it should be. We all want to progress this issue as swiftly as possible. But there is no doubt that a maintainer with limited time on their hands will gravitate towards RTBTC issues that appear to be reliably and thoroughly tested and reviewed.

In the other issue referred to above, the maintainer was narked off to find when he reviewed the code he had to give feedback because the code was not 'ready' yet. If you have substantial experience of reviewing code as good and then getting corroborated by maintainers I suspect you do) great! And you answered a question for me about the autofix after 2 weeks. It seems there is no autofix. A maintainer has to move to 'fixed' manually. Not sure why I see the message like 'auto-fixed after 2 weeks of no activity' though..

Yep fair points. I'm only becoming more involved in core development recently. I'm still learning. There definitely is a human element at play that can be the difference between how fast issues move. This issue will also get more eyes being tied to Navigation, which is tied to Drupal CMS, which has a lot of resources focused on it ATM. Unfortunately, these types of things are not always well articulated in documentation and otherwise.

In any case, thank you for your contribution and thoughtful comments. Hopefully we see this one through to completion soon!

It's not an exact science how to get a committer to look at an issue, you're always welcome to ping us on slack. we have the #core-development channel for that :)

Committed and pushed 9b3b1c6b552 to 11.x and 8e61da94f96 to 11.1.x. Thanks!

Thank you @nod_ I will take a look at the #core-development channel.