Problem/Motivation

hook_help() currently takes the $route_name and $request.
Now that we have the RouteMatch class, we're trying to remove direct usage of $request->attributes.

Proposed resolution

Replace the Request with RouteMatch.

Even though $route_name can be retrieved from RouteMatch, it is used in every single instance, and it's nice to have available.

This is an API change for any existing D8 modules, but will just supersede the previous API change of using route name and request instead of path, mitigating its affect on new modules.

Remaining tasks

Write patch

User interface changes

N/A

API changes

Signature of hook_help() will change again.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

effulgentsia’s picture

Priority: Normal » Critical
Issue tags: +beta blocker

This seems reasonable to me, and I think is one of the child issues that warrants the same priority and beta blocker status as #2281619: Convert most direct usages within module code of routing related request attributes to use RouteMatchInterface instead because it affects every contrib module. Note that in core, only 6 of 58 modules that implement hook_help() actually do anything with the 2nd parameter, but since our convention for hook implementations is to declare all hook parameters, regardless of if they're used, each one needs to be adjusted.

tim.plunkett’s picture

Status: Active » Needs review
FileSize
55.95 KB

Status: Needs review » Needs work

The last submitted patch, 2: 2294129-hook_help-2.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
59.98 KB
4.03 KB

Forgot it was invoked in 2 places.

Status: Needs review » Needs work

The last submitted patch, 4: 2294129-hook_help-4.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
63.09 KB
3.11 KB

Make that 3.

xjm’s picture

Does this really need to block beta? It's a one-parameter change to one hook. Very minor update to make, affects no data structures, and hook_help() is not one of the critical APIs.

xjm’s picture

https://www.drupal.org/node/2250345 will need an update for this change.

xjm’s picture

  1. +++ b/core/modules/help/src/Controller/HelpController.php
    -  public function helpMain(Request $request) {
    +  public function helpMain() {
    
    
    -      '#markup' => '<h2>' . $this->t('Help topics') . '</h2><p>' . $this->t('Help is available on the following items:') . '</p>' . $this->helpLinksAsList($request),
    +      '#markup' => '<h2>' . $this->t('Help topics') . '</h2><p>' . $this->t('Help is available on the following items:') . '</p>' . $this->helpLinksAsList(),
    
    
    -  protected function helpLinksAsList(Request $request) {
    +  protected function helpLinksAsList() {
    
    
    -  public function helpPage($name, Request $request) {
    +  public function helpPage($name) {
    

    Nice.

  2. +++ b/core/modules/system/src/Form/ModulesListForm.php
    @@ -95,14 +104,17 @@ public static function create(ContainerInterface $container) {
    -  public function __construct(ModuleHandlerInterface $module_handler, KeyValueStoreExpirableInterface $key_value_expirable, AccessManager $access_manager, EntityManagerInterface $entity_manager, QueryFactory $query_factory, AccountInterface $current_user) {
    +  public function __construct(ModuleHandlerInterface $module_handler, KeyValueStoreExpirableInterface $key_value_expirable, AccessManager $access_manager, EntityManagerInterface $entity_manager, QueryFactory $query_factory, AccountInterface $current_user, RouteMatchInterface $route_match) {
    
    @@ -211,7 +223,7 @@ protected function buildRow(array $modules, Extension $module, $distribution) {
    -      if ($this->moduleHandler->invoke($module->getName(), 'help', array('help.page.' . $module->getName(), $this->getRequest()))) {
    +      if ($this->moduleHandler->invoke($module->getName(), 'help', array('help.page.' . $module->getName(), $this->routeMatch))) {
    

    So the reason we're adding an additional injected parameter here but not removing one is that FormBase already provided the request object with getRequest(). Looking over the usages of this method in the form controllers in core, maybe half of them are route parameters, but the other half are legit uses of things related to the actual request (query parameters, etc.). So leaving the request in FormBase makes sense. (Would be out of scope here anyway; just making an note since I had to look into it a little.)

  3. +++ b/core/modules/system/src/Plugin/Block/SystemHelpBlock.php
    @@ -59,12 +66,15 @@ class SystemHelpBlock extends BlockBase implements ContainerFactoryPluginInterfa
    -  public function __construct(array $configuration, $plugin_id, $plugin_definition, Request $request, ModuleHandlerInterface $module_handler) {
    +  public function __construct(array $configuration, $plugin_id, $plugin_definition, Request $request, ModuleHandlerInterface $module_handler, RouteMatchInterface $route_match) {
    

    SystemHelpBlock still includes a need for the request because we need to check it to not show it on 403/404 pages (in getActiveHelp()).

  4. +++ b/core/modules/system/system.api.php
    @@ -997,7 +997,7 @@ function hook_permission() {
    - *   @link https://drupal.org/node/632280 the standard help template. @endlink
    + * @link https://drupal.org/node/632280 the standard help template. @endlink
    

    This is an incorrect change (the indentation was correct before), accidental maybe?

  5. +++ b/core/modules/system/system.api.php
    @@ -1005,14 +1005,14 @@ function hook_permission() {
    -function hook_help($route_name, \Symfony\Component\HttpFoundation\Request $request) {
    +function hook_help($route_name, \Drupal\Core\Routing\RouteMatchInterface $route_match) {
    

    I double-checked that the example code did not include a use of the request. (Maybe there should be an example? But separate docs issue.)

Did the following to review:

In the process of doing this I noticed datetime_help() still has a now-incorrectly-named $arg parameter, separate bug. I also noticed that there is a template_preprocess_file_upload_help() and an xmlrpc_server_method_help(); hopefully no one ever wants to make template_preprocess_file_upload.module or xmlrpc_server_method.module:P (Of course such would probably be full of other namespace collision risks anyway.)

I think this looks fine, other than the random whitespace change above.

xjm’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
62.63 KB
676 bytes

Fixing the whitespace thing. This is RTBC IMO (we can fix the change record after commit since it's just a little switcheroo).

xjm’s picture

FileSize
63.38 KB
776 bytes

Actually I started to change datetime_help() in a different patch but we should just fix it here since it is kinda in scope to update the second parameter.

Leaving at RTBC since it's just a teensy change.

xjm’s picture

Title: Consider switching hook_help() to use RouteMatch instead of Request » Switch hook_help() to use RouteMatch instead of Request

Still really don't think this is a beta blocker though.

webchick’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -beta blocker

Agreed. Seems simple enough though.

Committed and pushed to 8.x. Thanks!

  • webchick committed 94e0188 on 8.x
    Issue #2294129 by xjm, tim.plunkett: Switch hook_help() to use...
tim.plunkett’s picture

Status: Fixed » Closed (fixed)

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