Background: 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

Tasks:
- review / write the hook_help text according to help guidelines
- check if d.o docs exist / create (placeholder) d.o. docs https://drupal.org/documentation/modules/ban

Follow up:
#2055865: Revert file permission of ban.module file from 100755 back to 100644

Files: 
CommentFileSizeAuthor
#22 review-help-text-ban-1977644-22.patch908 bytesifrik
PASSED: [[SimpleTest]]: [MySQL] 56,632 pass(es).
[ View ]
#19 review-help-text-ban-1977644-19.patch904 bytesbatigolix
PASSED: [[SimpleTest]]: [MySQL] 57,022 pass(es).
[ View ]
#16 review-help-text-ban-1977644-16.patch899 bytesbatigolix
PASSED: [[SimpleTest]]: [MySQL] 56,855 pass(es).
[ View ]
#7 review-help-text-ban-1977644-7.patch878 bytesbatigolix
PASSED: [[SimpleTest]]: [MySQL] 56,366 pass(es).
[ View ]
#4 review-help-text-ban-1977644-4.patch1.21 KBbatigolix
PASSED: [[SimpleTest]]: [MySQL] 58,233 pass(es).
[ View ]

Comments

batigolix’s picture

Title:Update hook_help ban module» Update hook_help Ban module
Issue tags:+d8docs, +d8help

tagging

ifrik’s picture

In the current D8 dev version, a help text is displayed, but the configuration link just links to /admin/config/people - without any configuration for ban available.

batigolix’s picture

In my D8 version it links to http://example.com/admin/config/people/ban that leads to an actual UI

batigolix’s picture

Status:Active» Needs review
StatusFileSize
new1.21 KB
PASSED: [[SimpleTest]]: [MySQL] 58,233 pass(es).
[ View ]

I added a link to the online docs

batigolix’s picture

Component:ban.module» documentation

Changing component

jhodgdon’s picture

Status:Needs review» Needs work

Let's combine the first two sentences into a single paragraph, as in our hook_help standard.

Also, this patch should not remove the blank line here:

       return $output;
-

Other than that, and assuming someone has verified that the links work, I think it looks good!

batigolix’s picture

Status:Needs work» Needs review
StatusFileSize
new878 bytes
PASSED: [[SimpleTest]]: [MySQL] 56,366 pass(es).
[ View ]

I would like to keep the sentence below in its proper t function, so that it only needs to be translated once:

<?php
t
('For more information, see the <a href="!url">online documentation</a>.', array('!url' => 'https://drupal.org/documentation/modules/ban'))
?>
jhodgdon’s picture

That is a great idea. However, you can still make it part of the same paragraph... Ah, you did that here in your patch. Good! :)

Do you want to update the "how to create help" standards page with this formatting suggestion so people can continue to use the template there as a starting point?

Anyway, assuming this turns green, I think it's fine. I will wait to mark it "RTBC" until someone besides batigolix has verified the links work, since the first reviewer says they didn't work. Whoever is reviewing: start with a clean D8 installation and make sure the Ban module is enabled.

Status:Needs review» Needs work
Issue tags:-d8docs, -d8help

The last submitted patch, review-help-text-ban-1977644-7.patch, failed testing.

batigolix’s picture

Status:Needs work» Needs review
Issue tags:+d8docs, +d8help

#7: review-help-text-ban-1977644-7.patch queued for re-testing.

batigolix’s picture

Status:Needs review» Needs work

I changed the standards page: https://drupal.org/node/632280

ifrik’s picture

I've tested the patch in #7 on a clean 8.x-dev version.
Both links work and the text makes sense to me.

jhodgdon’s picture

Status:Needs work» Reviewed & tested by the community

Thanks for testing! In that case let's mark it RTBC. I think the "needs work" status change in #11 was a mistake.

YesCT’s picture

Issue tags:+RTBC July 1

This issue was RTBC and passing tests on July 1, the beginning of API freeze.

jhodgdon’s picture

Status:Reviewed & tested by the community» Needs work

In light of our discussion on other Help issues, let's fix the reference to the online documentation. I think the people working on this issue know what to do...

jhodgdon’s picture

Issue summary:View changes

style

batigolix’s picture

Status:Needs work» Needs review
StatusFileSize
new899 bytes
PASSED: [[SimpleTest]]: [MySQL] 56,855 pass(es).
[ View ]

Reverted the reference to the docs/handbook pages.

I think we prefer to refer to it as "online docs" nowadays.

jhodgdon’s picture

I think the text inside the A tags (the link text) should be "the online documentation for the Ban module" right?

jhodgdon’s picture

Status:Needs review» Needs work

Yeah, the link should be (link)online documentation for the Ban module(endlink) and the URL should also be https.

batigolix’s picture

Status:Needs work» Needs review
StatusFileSize
new904 bytes
PASSED: [[SimpleTest]]: [MySQL] 57,022 pass(es).
[ View ]

Attached patch:

- fixes text inside the A tags
- has https link

jhodgdon’s picture

OK, good!

Does this need a Uses section with a link to the admin page where this banning can be done and a brief explanation of how to do it?

jhodgdon’s picture

Status:Needs review» Reviewed & tested by the community

Doh! That Uses section is already there. :)

OK, I think this one is ready to be committed. It just adds the required link to the online docs.

ifrik’s picture

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new908 bytes
PASSED: [[SimpleTest]]: [MySQL] 56,632 pass(es).
[ View ]

The link was to a handbook page that didn't exist - I've changed it to the documentation page.
Remove trailing whitespace.

jhodgdon’s picture

Status:Needs review» Reviewed & tested by the community

Doh! Good catch. RTBC again...

jhodgdon’s picture

Status:Reviewed & tested by the community» Fixed

Thanks all! Committed to 8.x.

ParisLiakos’s picture

Issue tags:+Needs followup
diff --git a/core/modules/ban/ban.module b/core/modules/ban/ban.module
old mode 100644
new mode 100755

needs followup for this

jhodgdon’s picture

Status:Fixed» Needs work
ifrik’s picture

This already seem to be fixed in the current 8.x version.

jhodgdon’s picture

Status:Needs work» Fixed

Agreed. I just did a git pull on 8.x and the permissions are correct for all the files in core/modules/ban.

vijaycs85’s picture

Issue tags:-Needs followup

Created follow up for #25 at #2055865: Revert file permission of ban.module file from 100755 back to 100644 and removing corresponding tag.

vijaycs85’s picture

Issue summary:View changes

add link

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

Anonymous’s picture

Issue summary:View changes

Updated issue summary with follow up #2055865