Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhodgdon’s picture

Priority: Normal » Critical

Since there is currently no help for this module, in violation of the Documentation Gate, this is a critical issue.

batigolix’s picture

Status: Active » Needs review
FileSize
900 bytes

First attempt

Status: Needs review » Needs work

The last submitted patch, create-help-text-menu_link-2033413-2.patch, failed testing.

batigolix’s picture

Status: Needs work » Needs review
jhodgdon’s picture

Status: Needs review » Needs work

I think that this help text is fine. However, I am not sure about the links:

 '/admin/help/menu','@menu' => '/admin/structure/menu'))

a) You need to use the url() function to make internal links.
b) I don't think linking to admin/structure/menu with link text "Menu module" is a good idea. Normally within help we just link to the other module's help page, not the configure page. So probably we just need one link to the Menu module's help.

c) Also, modules are proper names so "Menu Link module" is the right name, not "Menu link module".

batigolix’s picture

Status: Needs work » Needs review
FileSize
849 bytes

attached patch:

- fixes a), b), c) from #5

i didnt know about using the url function for links. i probably made this mistake in plenty of other hook help texts as well...

Status: Needs review » Needs work

The last submitted patch, create-help-text-menu_link-2033413-6.patch, failed testing.

jhodgdon’s picture

Status: Needs work » Needs review
jhodgdon’s picture

Status: Needs review » Needs work

Thanks!

"It is required by the Menu module which provides an interface for managing menus." ==> needs a comma before "which".

Other than that, This text looks good to me.

I haven't seen any other url() problems but I'll keep an eye out. You only need url() for doing within-site links, not external links to drupal.org etc.

batigolix’s picture

Status: Needs work » Needs review
FileSize
850 bytes

patch adds comma

jhodgdon’s picture

Issue tags: +Needs manual testing

Looks good to me! Can someone who didn't write the patch please install and test? Check that the formatting is fine and that the link works. Then mark it "RTBC" if all is well. Thanks!

amateescu’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs manual testing

Tested manually and all is well :)

catch’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/menu_link/menu_link.moduleundefined
@@ -7,6 +7,16 @@
+      $output .= '<h3>' . t('About') . '</h3>';
+      $output .= '<p>' . t('The Menu Link module allows users to create menu links. It is required by the Menu module, which provides an interface for managing menus. See the <a href="@menu-help">Menu module help page</a> for more information.', array('@menu-help' => url('admin/help/menu'))) . '</p>'; ¶
+      return $output;
+  }

Trailing whitespace here.

amateescu’s picture

I let that slide because we have git apply --whitespace=fix :)

jhodgdon’s picture

That tool may exist, but that doesn't mean all maintainers use it when applying patches. I usually use the "patch" command to apply patches for docs, not git apply.

amateescu’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
849 bytes

Okay, let's not wait for a whitespace then :)

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

ParisLiakos’s picture

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

we need a followup for this

catch’s picture

Fixed locally and pushed. I usually spot those but apparently not this time...

ParisLiakos’s picture

Issue tags: -Needs followup

awesomeness, thanks!

Status: Fixed » Closed (fixed)

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

lostkangaroo’s picture

Priority: Critical » Normal
Status: Closed (fixed) » Needs work

Needs a reroll with updated links

batigolix’s picture

Status: Needs work » Needs review
FileSize
1.04 KB

Patch with updated links

batigolix’s picture

new patch that changes to @link to !link

jhodgdon’s picture

Should be fine... Can someone manually test to verify the link works in the latest Drupal 8?

jhodgdon’s picture

Issue tags: +Needs manual testing

tag

lostkangaroo’s picture

Status: Needs review » Reviewed & tested by the community

Manually Tested and works as intended. Two Thumbs up.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

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