Add new permission for accessing help pages

I fixed a typo in the issue summary for clarity.

We discussed this issue, and the related #2881856: Replace 'Use administration pages and help', at #3225198: Drupal Usability Meeting 2021-07-30.

We agree that it is a good idea to separate the permission for viewing help from the permission for accessing admin pages.

We also suggested that, before doing any more work on these issues, you check with the people working on the (currently experimental) Help Topics module. If they already have plans for changing the permissions related to viewing help pages, then you should try to coordinate with them. A good way to contact them is the #documentation channel in Drupal Slack.

Greeting from the Help Topics front! Thanks for reaching out in the #documentation Slack channel.

We do have a proposal for implementing per-help-topic permissions, similar to how per-views permissions are done, but the issue has been postponed. Here's the link:

https://www.drupal.org/project/2369943/issues/2951565

There was quite a lot of progress on it, so you might want to take a look at the work that had already been done in the comments of that issue.

For what's it's worth, I do think it's a great a idea to separate admin page permissions from help-related permissions, and given the long-term plan of having Help Topics replace hook_help(), I think one way to implement this is with per-help-topics permissions, as proposed in #2951565: Expand permissions for help topics.

Thanks for sharing the documentation perspective on this. In that case, I see two paths forward,

1. Resolve this issue with the latest patch's approach
2. Resolve this issue by replacing hook_help() with Help Topics and then separate the "Access administration pages" and "Access help pages" permissions from one another.

Is there an ETA on when Help Topics will replace hook_help()? Personally, I'm inclined to move ahead with the work to separate the two permissions now and then make further updates as necessary when Help Topics replaces hook_help().

Regarding Help Topics' progress, there is no "ETA", but you can see that there are only a few issues remaining in both meta issues: #3027054: Help Topics module roadmap: the path to beta and stable and #3041924: [META] Convert hook_help() module overview text to topics.

Right on, thanks for the update.

We discussed this issue and the related Help Topics work in the UX meeting today. We concluded that this work is decoupled from the Help Topics work enough to safely move forward with completing the work to separate the view admin pages permission from the view help pages permission.

In that case, RoSk0, if you can update your patch to pass automated tests I'd be happy to review the new patch and garner wider support to move this issue forward.

Closed as duplicate #2881856-14: Replace 'Use administration pages and help'

This permission is needed since #3090659: Make a way for help topics to generate links only if they work and are accessible but meantime conversion is in progress in #3215784: [META] Fix up topics to use new help_route_link function

Patch fixes upgrade path, now only test fixes and new test for upgrade is missing

Basically permission should be provided by help module and when help topics will be merged into help then no roles will need to update

Added upgrade test

Fix message for existing permission

Fix some tests

As tests are green, it needs to review replacement and unification of usage for new permission

Think all that's missing for this is a change record for the new permission.

Added CR https://www.drupal.org/node/3344060

Thanks @andypost!

Reviewed Patch on #25, seems issue has been fixed.

Tested and works as expected, hence generate MR. Please review.

@Rashmisoni, thanks for your interest in contributing to the issue.
The work is already being done in a patch and the issue is already marked RTBC, so opening a new merge request is redundant and just adds noise.
Therefore, I've removed issue credit for the addition of this merge request, and asked the committer to close it.
In the future, you can contribute to issues by working on them before they are RTBC.
Thanks!

The change record can be improved. I looked at the following CRs that are also about adding a new permission and they have more details. Let's do that here. The last one has a screenshot which is helpful. Let's make sure the grammar is correct.

• https://www.drupal.org/node/3346838
• https://www.drupal.org/node/3280750
• https://www.drupal.org/node/3070293

I read the patch and noticed the following.

1. +++ b/core/modules/help/help.post_update.php
   @@ -0,0 +1,22 @@
   + * Post update functions for Help.
   

   Why is Help capitalized? Should this be 'Help module?

2. +++ b/core/modules/help/tests/fixtures/update/drupal-10.permission-3204810.php
   @@ -0,0 +1,27 @@
   + * Contains database additions to for testing upgrade path for help permission.
   

   This should be standard English. This reads like there is only one help permission. Is that true?

   See the standard for @file, @file: Documenting files.

3. Why is this adding issue numbers to files?

   +++ b/core/modules/help/tests/fixtures/update/drupal-10.permission-3204810.php
   @@ -0,0 +1,27 @@
   + * @see https://www.drupal.org/node/3204810
   

   Not needed.

4. +++ b/core/modules/system/tests/src/Functional/Module/InstallUninstallTest.php
   @@ -84,6 +84,9 @@ public function testInstallUninstall() {
   +    // Add new role to testing user to access help pages.
   

   I have not read the whole test. Should this be 'test user'?

Yes, 'access help pages' the only new permission

@andypost, thanks.

+++ b/core/modules/help/src/Annotation/HelpSection.php
--- /dev/null
+++ b/core/modules/help/tests/fixtures/update/drupal-10.permission-3204810.php
--- /dev/null
+++ b/core/modules/help/tests/fixtures/update/drupal-10.permission-3204810.yml

Another thing I noticed is the use of an issue number in these file names. I have not seen that before and at first I thought it could be helpful but then one can just use git blame. And as I thought about it I would prefer a descriptive title. Maybe 'access-help-pages'?

I found that there is precedent of this in core, in CkEditor5 and I also found that the coding standards for filenames does not address this situation. So, I asked the other committers. There was sympathy for using issue number because naming can be hard. However, xjm was quite clear that this is an anti-pattern and the only d.o issue IDs in the codebase should be for followups (@todos) and for change records. Those are good points, so let's go with that.

Lets change the fixture filenames. Thanks.

Updated CR, added screenshot

patch fixes #36 2-3-4 comments but 1 is core's standard for post update files

Re #39 I'm using this naming since https://git.drupalcode.org/project/drupal/-/tree/9.5.x/core/modules/acti...

Changed filenames to drupal-10.access-help-pages.* according to https://git.drupalcode.org/project/drupal/-/tree/10.1.x/core/modules/sys...

@andypost, ping me in Slack that this was ready for a review. I applied the patch and tested it. And then from that I tweaked the change record. It was good to see the screenshot added to the CR, I think that does help the user. I pinged back asking andypost to check the CR changes. They responded that the changes look better.

I checked the patch changes and agree that everything in #36 and # 38 have been addressed. I am setting this back to RTBC.

Known random failure

Unfortunately this is too late to land in 10.1.x but can go into 11.x for release in 10.2.0.

Another random failure #3362597: [random test failure] ContextualLinksTest::testContextualLinksClick

+++ b/core/modules/help/help.post_update.php
@@ -0,0 +1,22 @@
+function help_post_update_add_permissions_to_roles(?array &$sandbox = NULL): void {

Is the default type-hint correct here?

ConfigEntityUpdater::update expects an array for sandbox, so we can't default this to NULL in my book.

Additionally, should we be doing anything with ::blockAccess on the help block? Currently it has no permissions, so I guess not.

Filed follow-up to polish sandbox argument as I used to copy it from common-wrong-place #3370322: Set default value = [] for sandbox argument of update hooks

We don't provide blockAccess() to allow end-user to decide, by default the block is limited by /help path

Re-rolled patch and fixed #48

Re-roll was incomplete

Moving back to RTBC. Will review the follow up now hopefully can land early for 10.2

Committed 680039f and pushed to 11.x. Thanks!

Updated the change record to reference 10.2.0 and published it.

Thanks folks

What a great new feature, thanks!

Drupal gets better every day, and it's a joy to follow the Change record page. There has been 126 Change record announcements in the first 180 days of this year, ~0.7 per day.