Performance regression in contact_help()

This was found while pair-profiling with @dawehner, adding a suggested commit message to the IS.

Not sure how we can do that (regression test), but this would be RTBC from my side.

Maybe a sub-classed UrlGenerator inserted into the Container in a web test, then calling $moduleHandler->invokeAll('help')?

Here is a test ... the image is a prove that it actually works.

I'm sorry, totally forgot the actual files

The regression test should be a static analysis that hook_help implementations should only have whitespace (or comments) between the opening bracket and the switch statement. It is pointless to guard against each possible mistake.

I've created a coder code sniffer feature request about that.

1. +++ b/core/modules/help/src/Tests/HelpEmptyPageTest.php
   @@ -0,0 +1,65 @@
   +   * Ensures that no hook_help() is called on a page without hook_help().
   

   I think we're testing that there's no routing activity, no?

2. +++ b/core/modules/help/src/Tests/HelpEmptyPageTest.php
   @@ -0,0 +1,65 @@
   +    \Drupal::service('module_handler')->invokeAll('help', ['<front>', new RouteMatch('<front>', $route)]);
   

   The standard way to do this is a try / catch block with $this->fail() and $this->pass() :)

Muh.

@amateescu took back point 2.

1. +++ b/core/modules/help/tests/modules/help_test/src/SuperNovaGenerator.php
   @@ -0,0 +1,74 @@
   +class SuperNovaGenerator implements UrlGeneratorInterface {
   

   :D

2. +++ b/core/modules/user/user.module
   @@ -1136,11 +1136,12 @@ function user_role_change_permissions($rid, array $permissions = array()) {
    function user_role_grant_permissions($rid, array $permissions = array()) {
      // Grant new permissions for the role.
   -  $role = Role::load($rid);
   -  foreach ($permissions as $permission) {
   -    $role->grantPermission($permission);
   +  if ($role = Role::load($rid)) {
   +    foreach ($permissions as $permission) {
   +      $role->grantPermission($permission);
   +    }
   +    $role->save();
      }
   -  $role->save();
    }
   

   Seems unrelated?

Other than that, looks ready.

Seems unrelated?

Well, a) its the right thing to do, certainly b) its causing failures in the kernel test, if you don't check it.

Alright, then this is ready!

I agree, RBTC +1 :)

RTBC++

This needs a comment in hook_help() to explain why we build the links in two places. The test will stop a patch from getting committed, but I can see it being confusing and people trying to revert this.

Comment added.

Looks like Alex got this one.

Thanks testbot.

Tiny follow-up #2476745: Fix case of SuperNovaGenerator in HelpEmptyPageTest