Add new permission for accessing help pages

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

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

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

The last submitted patch, 20: 3204810-20.patch, failed testing. View results →

The last submitted patch, 21: 3204810-21.patch, failed testing. View results →

Fix some tests

The last submitted patch, 22: 3204810-22.patch, failed testing. View results →

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.

Rashmisoni → made their first commit to this issue’s fork.

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.

The last submitted patch, 40: 3204810-40.patch, failed testing. View results →

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.

The last submitted patch, 40: 3204810-40.patch, failed testing. View results →

Another random failure 🐛 [random test failure] ContextualLinksTest::testContextualLinksClick Closed: outdated

+++ 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 📌 Set default value = [] for sandbox argument of update hooks Needs work

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

The last submitted patch, 49: 3204810-49.patch, failed testing. View results →

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.

Automatically closed - issue fixed for 2 weeks with no activity.