Refactor certain inline module-exists checks in core components for readability

This is mostly great! I think we should make sure to do a manual test of all the changed help and other pages before we mark this RTBC and commit it, to make sure all the links are working.

A few comments, and one thing to fix:

1. +++ b/core/modules/aggregator/aggregator.module
   @@ -20,6 +20,7 @@
   +      $block_page = \Drupal::moduleHandler()->moduleExists('block') ? \Drupal::url('block.admin_display') : '#';
   

   Since $block_page is already needed in two places in this same function, how about defining the variable outside the case statement, rather than doing it twice, one in each case?

   Well, I guess that would be marginally less efficient for the cases that don't need it. So, either way. For better maintainability and because I think the efficiency hit is very small (and I hate copy/paste of code in general), I would be in favor of moving it outside the switch, but we could go either way.

2. +++ b/core/modules/field/field.module
   @@ -64,10 +64,10 @@
   -      $field_ui_url = \Drupal::moduleHandler()->moduleExists('field_ui') ? \Drupal::url('help.page', array('name' => 'field_ui')) : '#';
   +      $field_ui_page = \Drupal::moduleHandler()->moduleExists('field_ui') ? \Drupal::url('help.page', array('name' => 'field_ui')) : '#';
          $output = '';
   

   This looks suspicious, but I verified that $field_ui_url is not being used anywhere, so it's fine.

3. +++ b/core/modules/help/help.api.php
   @@ -46,7 +46,8 @@ function hook_help($route_name, \Drupal\Core\Routing\RouteMatchInterface $route_
        case 'help.page.block':
   -      return '<p>' . t('Blocks are boxes of content rendered into an area, or region, of a web page. The default theme Bartik, for example, implements the regions "Sidebar first", "Sidebar second", "Featured", "Content", "Header", "Footer", etc., and a block may appear in any one of these areas. The <a href="!blocks">blocks administration page</a> provides a drag-and-drop interface for assigning a block to a region, and for controlling the order of blocks within regions.', array('!blocks' => \Drupal::url('block.admin_display'))) . '</p>';
   +      $block_page = \Drupal::moduleHandler()->moduleExists('block') ? \Drupal::url('block.admin_display') : '#';
   +      return '<p>' . t('Blocks are boxes of content rendered into an area, or region, of a web page. The default theme Bartik, for example, implements the regions "Sidebar first", "Sidebar second", "Featured", "Content", "Header", "Footer", etc., and a block may appear in any one of these areas. The <a href="!blocks">blocks administration page</a> provides a drag-and-drop interface for assigning a block to a region, and for controlling the order of blocks within regions.', array('!blocks' => $block_page)) . '</p>';
    
   

   In this file, we should not be making this change.

   This is the sample function body for the hook_help() hook, and the example it's using is for the Block module, so we do not need to check whether Block is enabled here.

Have made changes as per #7.3 and manually tested the help pages. Everything looks good to me.

Apologies @opratr. I realized that a bit late and in fact sent you an apology message as well. Never meant to work on an issue assigned to someone. Was a miss from my side. All in good faith!

Great, meant in good faith I'm sure. We all have to learn the procedures at some point. :) Anyway, the manual testing is much appreciated @umarzaffer, and we needed a new patch one way or another. Thanks for the gentle handling of the problem @opratr.

One way or another, the new patch looks good. Thanks!

Back to previous status after green retest.

Thanks everyone for your work here!

1. My first reaction to this issue is that it needs a beta evaluation and the case for making the change during the beta is not clear. See the allowed beta changes policy. The patch is not disruptive for contrib or existing sites, but it is the type of patch that needs frequent rerolls, takes disproportionate time to review, and may introduce the need for non-trivial rerolls in other patches. Furthermore, even if there were none of this disruption, any normal task with no prioritized changes should be postponed unless there is a special circumstance.
    

2. Leaving aside my first point and looking at the patch:

   +++ b/core/modules/aggregator/aggregator.module
   @@ -44,9 +45,10 @@ function aggregator_help($route_name, RouteMatchInterface $route_match) {
   +      $block_page = \Drupal::moduleHandler()->moduleExists('block') ? \Drupal::url('block.admin_display') : '#';
   

   We're repeating this line a lot (maybe 26 times or so based on the diffstat of this patch?). That would suggest to me that we should refactor this into a method (or a function in help.module even). That said though...
    

3. I do not think linking to # is the correct behavior. A link that basically does nothing is a usability and accessibility problem. It is also not semantic for a resource that does not exist. (See my comment in #2498675-5: Linking to a non-existent route should not throw an exception.)
    

I recognize for point 3 is not introduced by this patch. However, it would be reason to mark this issue wontfix, and we could instead improve this as a part of fixing that bug.

Oh, I forgot to mention that I discussed this with @alexpott as well.

I think that either the entire sentence should be conditionally displayed depending on whether or not the module is available, or the help text should be written in such a way that it's relevant whether or not the module is enabled (and possibly even informs you if the module is not available). @alexpott preferred the second option.

Another issue though is that even if the module is enabled, the user might not have permission to access the linked resource. I believe in some places we do a permission check and add the link or paragraph conditionally based on that as well (or we have in the past). At least, there have been conditionals like this in hook_help() implementations for sure.

We *want* the sentence to be there, actually, because it would tell people "You could enable this other module to get this functionality". I agree linking to # is not ideal.

Probably then ideally what we want to do is make the substitution into the t() function either be <a href="...">Block list page</a> or just Block list page. However I think this would be very difficult for translators if not impossible.

So I think we should leave things as they are.

Removing the Novice tag as this issue is lacking a consensus.
https://www.drupal.org/core-mentoring/novice-tasks#avoid