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.
Comment | File | Size | Author |
---|---|---|---|
#11 | interdiff-10-11.txt | 776 bytes | xjm |
#11 | help-2294129-11.patch | 63.38 KB | xjm |
#10 | interdiff-6-10.txt | 676 bytes | xjm |
#10 | help-2294129-10.patch | 62.63 KB | xjm |
#6 | interdiff.txt | 3.11 KB | tim.plunkett |
Comments
Comment #1
effulgentsia CreditAttribution: effulgentsia commentedThis 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.
Comment #2
tim.plunkettComment #4
tim.plunkettForgot it was invoked in 2 places.
Comment #6
tim.plunkettMake that 3.
Comment #7
xjmDoes 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.Comment #8
xjmhttps://www.drupal.org/node/2250345 will need an update for this change.
Comment #9
xjmNice.
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.)
SystemHelpBlock still includes a need for the request because we need to check it to not show it on 403/404 pages (in
getActiveHelp()
).This is an incorrect change (the indentation was correct before), accidental maybe?
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:
git diff --color-words
.which no longer has any results.
$request
parameter in HEAD. The following use it:All are covered by the patch.
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 maketemplate_preprocess_file_upload.module
orxmlrpc_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.
Comment #10
xjmFixing the whitespace thing. This is RTBC IMO (we can fix the change record after commit since it's just a little switcheroo).
Comment #11
xjmActually 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.
Comment #12
xjmStill really don't think this is a beta blocker though.
Comment #13
webchickAgreed. Seems simple enough though.
Committed and pushed to 8.x. Thanks!
Comment #15
tim.plunkettUpdated https://www.drupal.org/node/2250345/revisions/view/7392223/7393137