Goal
Review / write the hook_help for node module.
Skills needed
- Module Development
Detailed steps
Implement the hook_help following the help guidelines
Background and reference information
This issue is part of the task to update the hook_help texts of the Drupal 8 modules: #1908570: [meta] Update or create hook_help() texts for D8 core modules
Next steps: moving beyond this task
Comment | File | Size | Author |
---|---|---|---|
#25 | interdiff.txt | 3.09 KB | batigolix |
#15 | interdiff.diff | 3.09 KB | batigolix |
#15 | 2091343-hook-help-node-15.patch | 5.93 KB | batigolix |
#12 | 2091343-hook-help-node-12.patch | 5.95 KB | babruix |
#4 | 2091343-hook-help-node-3.patch | 5.93 KB | babruix |
Comments
Comment #1
jhodgdonifrik: this looks like you were attempting to set up a task rather than an issue? Normally we don't put steps, goals, skill level, etc. on issues.
Comment #2
babruix CreditAttribution: babruix commentedComment #3
ifrik@jhodgdon: I'm not quite sure where those goals etc come from. But yes this should simply be an issue like all the other ones.
Comment #4
babruix CreditAttribution: babruix commentedPatch to change tokens from @ to !.
Also url changed to \Drupal::url
Comment #5
jhodgdonThanks! Sorry this took so long to review -- I'm still behind from the Prague sprint.
This looks like it is probably OK. We need someone to do a manual test to make sure all the links still work. We also need someone to manually test whether all the text and instructions in the help still match what you see in the Drupal 8 user interface.
Comment #6
jhodgdon#4: 2091343-hook-help-node-3.patch queued for re-testing.
Comment #8
jhodgdon#4: 2091343-hook-help-node-3.patch queued for re-testing.
Comment #10
ifrikThere seem to be some error in the routing. Applying the patch breaks my site.
Comment #11
jhodgdonThat's odd. The PHP syntax in the patched file node.module is fine.
Comment #12
babruix CreditAttribution: babruix commentedProblem with routing was that I`ve tried to generate URL for path admin/content using
\Drupal::url('view.content.page_1')
which seems to be incorrect.If someone could advise how to generate URL by using routing name for Views path 'admin/content' would be nice to know.
For now using \Drupal::urlGenerator()->generateFromPath('admin/content') for that.
Comment #14
jhodgdonRE #12 - look at node.routing.yml - you want node.content_overview for /admin/content.
Comment #14.0
jhodgdonCreate documentation patch
Comment #15
batigolixpatch:
- http > https
- reference to online docs according to standard
- fix url() mentioned in #12
Comment #17
jhodgdonI don't see any problems... looks pretty straightforward and I don't think anything really needs to be changed.
So I think this one is ready for a quick manual test:
- Verify that all the links work
- Verify that all mentions of pages/text within the UI match what is seen in the UI
- Verify that the formatting is OK.
Comment #18
lostkangaroo CreditAttribution: lostkangaroo commentedBased on the manual test this still needs a bit of work.
user_access() is marked as depreciated so it should probably be replaced
A number of the cases didn't render any help text
I am assuming the % isn't being converted to the content type or nid depending. An article was created to conduct type and node testing of the pages that failed.
All links worked and text reads well.
Comment #19
jhodgdonMost of #18 is probably due to:
#2180343: hook_help's $path doesn't contain '%' wildcards.
and would probably be fixed by
#2183113: Update hook_help signature to use route_name instead of path
so I think we should just leave those for the moment.
This issue is purely about fixing up the help *text* anyway, so we should probably file a follow-up issue to address coding issues. Are there any text issues we need to fix? I wasn't sure what "user_access() is marked as depreciated so it should probably be replaced" referred to as it is not in the patch.
Comment #20
lostkangaroo CreditAttribution: lostkangaroo commentedthe user_access actually occurs earlier in the code than the patch, around line 87 of node.module as of my pull this morning.
The text itself is fine in the patch as can be read but can not be fully manually tested until the cases are fixed. I am just wary of breaking things by adding a patch that fails to do what it is supposed to do. Even though this is probably the case now for most of the other hook helps as well.
Comment #21
jhodgdonGood point about the manual testing... I don't think this applies to most help text in Core, because actually very few of the paths have % in them, and other paths are working fine.
So I see what you're saying about the top part of hook_help(). It is calling user_access() as well as url(), both of which are deprecated... Let's have a follow-up issue to fix that part.
Regarding the main module help text (case 'admin/help#node'), which is all that this patch actually addresses, that has been reviewed and tested, so let's go ahead and get this patch in.
And then the follow-up issue should also review the help fragments with paths containing %.
Here's the follow-up issue:
#2191133: Fix up/review additional parts of node module help
Comment #22
lostkangaroo CreditAttribution: lostkangaroo commentedExcellent I agree RTBC
Comment #23
alexpottinterdiff.diff no longer applies.
Comment #24
alexpottSorry an automated tool thought that interdiff.diff was the correct patch for the issue. Interdiffs should be posted as .txt files to avoid testbot wasting time on them.
Comment #25
batigolixsorry for that
yeah due to a brainlapse i posted a .diff file
attached is the same file w .txt extension
Comment #26
alexpottCommitted 1ec781d and pushed to 8.x. Thanks!
@batigolix I've done the same thing more than once! I've fixed the automated tool to detect this :)