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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhodgdon’s picture

ifrik: 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.

babruix’s picture

Assigned: Unassigned » babruix
Issue tags: +prague 2013
ifrik’s picture

@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.

babruix’s picture

Assigned: babruix » Unassigned
Status: Active » Needs review
Issue tags: -prague 2013
FileSize
5.93 KB

Patch to change tokens from @ to !.
Also url changed to \Drupal::url

jhodgdon’s picture

Issue tags: +Needs manual testing

Thanks! 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.

jhodgdon’s picture

Issue tags: -Needs manual testing

#4: 2091343-hook-help-node-3.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 2091343-hook-help-node-3.patch, failed testing.

jhodgdon’s picture

Status: Needs work » Needs review

#4: 2091343-hook-help-node-3.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Needs manual testing

The last submitted patch, 2091343-hook-help-node-3.patch, failed testing.

ifrik’s picture

There seem to be some error in the routing. Applying the patch breaks my site.

jhodgdon’s picture

That's odd. The PHP syntax in the patched file node.module is fine.

babruix’s picture

Status: Needs work » Needs review
FileSize
5.95 KB

Problem 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.

Status: Needs review » Needs work

The last submitted patch, 2091343-hook-help-node-12.patch, failed testing.

jhodgdon’s picture

RE #12 - look at node.routing.yml - you want node.content_overview for /admin/content.

jhodgdon’s picture

Issue summary: View changes

Create documentation patch

batigolix’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
5.93 KB
3.09 KB

patch:

- http > https
- reference to online docs according to standard
- fix url() mentioned in #12

Status: Needs review » Needs work

The last submitted patch, 15: interdiff.diff, failed testing.

jhodgdon’s picture

Status: Needs work » Needs review
Issue tags: +Novice

I 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.

lostkangaroo’s picture

Status: Needs review » Needs work

Based 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

  • admin/structure/types/manage/%/form-display
  • admin/structure/types/manage/%/display
  • node/%/revisions
  • node/%/edit

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.

jhodgdon’s picture

Most 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.

lostkangaroo’s picture

the 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.

jhodgdon’s picture

Status: Needs work » Reviewed & tested by the community

Good 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

lostkangaroo’s picture

Excellent I agree RTBC

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

interdiff.diff no longer applies.

error: patch failed: core/modules/node/node.module:97
error: core/modules/node/node.module: patch does not apply

alexpott’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs reroll

Sorry 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.

batigolix’s picture

FileSize
3.09 KB

sorry for that

yeah due to a brainlapse i posted a .diff file

attached is the same file w .txt extension

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 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 :)

Status: Fixed » Closed (fixed)

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