Background:
This issue is part of the task to update/create the hook_help texts of the modules for Drupal 8:
#1908570: [meta] Update or create hook_help() texts for D8 core modules

Tasks:
- review and update hook_help
- review d.o. docs at http://drupal.org/documentation/modules/menu

Files: 
CommentFileSizeAuthor
#32 interdiff-2090525-29-32.txt3.43 KBvisabhishek
#30 drupal.update-hook_help-for-menu-module.2090525-30.patch5.09 KBvisabhishek
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,738 pass(es).
[ View ]
#28 interdiff-2090525-18-28.txt3.4 KBellishettinga
#28 drupal-menu-2090525-28.patch5.16 KBellishettinga
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,245 pass(es).
[ View ]
#18 interdiff.txt2.17 KBbatigolix
#18 drupal8.menu-module.2090525-18.patch4.99 KBbatigolix
FAILED: [[SimpleTest]]: [MySQL] 64,409 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#14 interdiff.txt1.86 KBbatigolix
#12 interdiff.diff1.86 KBbatigolix
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch interdiff_0.diff. Unable to apply patch. See the log in the details link for more information.
[ View ]
#12 drupal8.menu-module.2090525-11.patch4.96 KBbatigolix
PASSED: [[SimpleTest]]: [MySQL] 63,404 pass(es).
[ View ]
#9 interdiff.txt4.17 KBbatigolix
#9 drupal8.menu-module.2090525-9.patch4.68 KBbatigolix
PASSED: [[SimpleTest]]: [MySQL] 63,145 pass(es).
[ View ]
#6 interdiff.txt4.37 KBbatigolix
#6 drupal8.menu-module.2090525-6.patch4.45 KBbatigolix
PASSED: [[SimpleTest]]: [MySQL] 62,937 pass(es).
[ View ]
#1 drupal8.menu-module.2090525-1.patch4.77 KBlostkangaroo
PASSED: [[SimpleTest]]: [MySQL] 58,845 pass(es).
[ View ]

Comments

lostkangaroo’s picture

StatusFileSize
new4.77 KB
PASSED: [[SimpleTest]]: [MySQL] 58,845 pass(es).
[ View ]

Patch updating links to current standards.

lostkangaroo’s picture

Issue tags:+Help text
lostkangaroo’s picture

Status:Active» Needs review
jhodgdon’s picture

Status:Needs review» Needs work

Do you think we should move the information about how to display menus out of About and into Uses? Actually, we already have that information in Uses, so maybe the sentence about blocks can just be removed?

Also the line in About that says to use the admin page to create and manage your menus can probably be removed?

The "see the online handbook" reference also needs to be updated to our current template.

I'm also wondering about "you must enable and position the associated block"... I think the word "must" is a bit strong here. There are other ways to display menus. How about removing "you must"? And if we're taking the information about blocks out of About, maybe add a sentence here saying that each menu becomes a block?

Thanks!

jhodgdon’s picture

Issue summary:View changes

Updated issue summary.

batigolix’s picture

The patch in #1 needs to be re-rolled

batigolix’s picture

Issue summary:View changes
Parent issue:» #1908570: [meta] Update or create hook_help() texts for D8 core modules
StatusFileSize
new4.45 KB
PASSED: [[SimpleTest]]: [MySQL] 62,937 pass(es).
[ View ]
new4.37 KB

Patch addresses point made in #6

Furthermore I reviewed (and adapted where necessary) UI page names & elements and I checked the links.

I worked around my meager patch re-rolling skills by manually applying the changes in patch #1. I hope nothing broken and no-one got hurt.

batigolix’s picture

Status:Needs work» Needs review
jhodgdon’s picture

Status:Needs review» Needs work

Looking pretty good! A few minor things to fix:

a) The t() in About has a couple of unused URLs in it, so those can be removed:

'!blocks' => \Drupal::url('block.admin_display'), '!menus' => \Drupal::url('menu.overview_page'),

b) First "Uses" item:

add, edit and delete custom menus

Serial comma needed before "and".

c) I noticed that on the Add page, we have:

     case 'admin/structure/menu/add':

But on the overview page, we have:

   if ($path == 'admin/structure/menu' && \Drupal::moduleHandler()->moduleExists('block')) {

In both cases, help is being added that refers to the blocks, so in both cases we should be checking to see if the block module is enabled (as is done in the second case but not the first case).

So, the first one should probably be done like the second one is: it needs to be moved outside the switch {} and be in an if() -- and one of them should be an elseif. Like this:

   if ($path == 'admin/structure/menu/add' && \Drupal::moduleHandler()->moduleExists('block')) {
   ...
   }
   elseif ($path == 'admin/structure/menu' && \Drupal::moduleHandler()->moduleExists('block')) {
   ...
   }

Hope that makes sense.

d) In the Displaying Menus uses item... Some themes automatically display certain menus -- do you think it would make sense to mention this? For instance, I think Bartik automatically displays the Main Menu in the header, or at least it used to in 7. Many other themes do too. I think it makes sense to mention this? It may be complicated by ... well, I'm not sure if you use the Minimal install profile if the Main menu is created for you or not.

e) Come to think of it, the first Uses item says that menus will automatically have blocks. Should we say "if you have the Blocks menu installed" and link to the blocks menu help there?

batigolix’s picture

Status:Needs work» Needs review
StatusFileSize
new4.68 KB
PASSED: [[SimpleTest]]: [MySQL] 63,145 pass(es).
[ View ]
new4.17 KB

a), b), c) and e) from #8 fixed

regarding #8 d) Do you mean that Bartik prints the main menu in the template? It still does that it seems.
The UI lets you enable, disable it, but it cannot be assigned to another region. Do we need to mention something like that you think?

jhodgdon’s picture

Yes, that is what I was talking about in #8 d. It seems relevant -- it's not just Bartik -- many themes do this (even Stark perhaps, and most contrib themes). So some mention of this seems like a good idea?

I also don't think we want a comma in:
"Each menu that you create, is rendered in a block"

Other than that, looks good!

jhodgdon’s picture

Status:Needs review» Needs work
batigolix’s picture

Status:Needs work» Needs review
StatusFileSize
new4.96 KB
PASSED: [[SimpleTest]]: [MySQL] 63,404 pass(es).
[ View ]
new1.86 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch interdiff_0.diff. Unable to apply patch. See the log in the details link for more information.
[ View ]

I added a sentence about the theme menu's

Fixed the comma. I will never get used to comma usage in English ;)

Status:Needs review» Needs work

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

batigolix’s picture

StatusFileSize
new1.86 KB

interdiff file with correct extension

batigolix’s picture

Status:Needs work» Needs review
Issue tags:+Novice
jhodgdon’s picture

Apparently, apostrophes as well as commas. :) In English, "menu's" should not have an apostrophe in:

In some themes the main and secondary menu are printed in the template. These menu\'s can be disabled via the theme\'s settings page.

Actually, how about if this gets changed to say:

In some themes, the main menu and possibly the secondary menu will be output automatically; you may be able to disable this behavior on the theme\'s settings page.

The reason: I don't know of too many themes that automatically output the secondary menu, and I'm also not sure that all themes that do output main or secondary menus automatically allow it to be disabled?

jhodgdon’s picture

Status:Needs review» Needs work
batigolix’s picture

Status:Needs work» Needs review
StatusFileSize
new4.99 KB
FAILED: [[SimpleTest]]: [MySQL] 64,409 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
new2.17 KB

Apostrophe *and* comma usage in English. I doesn't get it :(

Not sure if words like "possibly" and " you may be able" make the text easier to understand but yes, it is correct. Patch contains your proposed text from #16

Status:Needs review» Needs work

The last submitted patch, 18: drupal8.menu-module.2090525-18.patch, failed testing.

jhodgdon’s picture

jhodgdon’s picture

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

OK, assuming the tests pass this time, the patch looks good. The test failure didn't look totally unrelated to this patch, so I hit retest to make sure.

So this is ready for a manual test:
- Verify that all the links work
- Verify that all mentions of pages/text/permissions within the UI match what is seen in the UI
- Verify that the formatting is OK.

Status:Needs review» Needs work

The last submitted patch, 18: drupal8.menu-module.2090525-18.patch, failed testing.

batigolix’s picture

[14-Feb-2014 17:48:18 UTC] Uncaught PHP Exception Symfony\Component\Routing\Exception\RouteNotFoundException: "Route "block.admin_display" does not exist." at /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Routing/RouteProvider.php line 143

Maybe the route name has been changed?

I ll check

batigolix’s picture

Cannot reproduce this locally.

jhodgdon’s picture

I think what is happening is that the Block module used to be required, and now it isn't.

So the main help page for the Menu module is attempting to find the URL of the Blocks page with \Drupal::url('block.admin_display'), and this is causing an exception if the Block module is not enabled, which it apparently isn't in the test.

Sigh. I guess we need to either:
a) Not link to this admin page directly in the help or
b) Put in an if statement that only attempts the link if the block module is enabled.

To do (b), we would replace

!blocks' => \Drupal::url('block.admin_display'),

with

!blocks' => (\Drupal::moduleHandler()->moduleExists('block')) ? \Drupal::url('block.admin_display') : '#',
ellishettinga’s picture

Status:Needs work» Needs review
StatusFileSize
new4.97 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,286 pass(es).
[ View ]

Rerolled the patch and applied latest comments on it.
Affected are help texts on:
- admin/help#menu
- admin/structure/menu/add
- admin/structure/menu

jhodgdon’s picture

Thanks... Can you please make an interdiff from the previous patch, so that we can look at the changes you made specifically?

ellishettinga’s picture

StatusFileSize
new5.16 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,245 pass(es).
[ View ]
new3.4 KB

Ok here's a new version of the patch (I saw I missed something in the one I created yesterday) + the interdiff.

The Block module is installed by default now and I cannot get it disabled. This makes it difficult to test manually what happens if Block module is disabled.

jhodgdon’s picture

Status:Needs review» Needs work

Modules can no longer be disabled, but you should be able to "Uninstall" the Block module.

So... This looks pretty good! A few notes:

a) In the "Displaying menus" section, I think we should say "if you have the Block module enabled" somewhere in the sentence that says that each menu gets into a block that you can place etc.

b) In this code:

+  if ($path == 'admin/structure/menu/add' && \Drupal::moduleHandler()->moduleExists('block')) {
+      return '<p>' . t('You can enable the newly-created block for this menu on the <a href="!blocks">Block layout page</a>.', array('!blocks' => (\Drupal::moduleHandler()->moduleExists('block')) ? \Drupal::url('block.admin_display') : '#')) . '</p>';
+  }
+  elseif ($path == 'admin/structure/menu' && \Drupal::moduleHandler()->moduleExists('block')) {
+    return '<p>' . t('Each menu has a corresponding block that is managed on the <a href="!blocks">Block layout page</a>.', array('!blocks' => (\Drupal::moduleHandler()->moduleExists('block')) ? \Drupal::url('block.admin_display') : '#')) . '</p>';
   }

The if and elseif conditions already are checking moduleExists('block'), so we do not need to check it again in the return lines.

Thanks!

visabhishek’s picture

Status:Needs work» Needs review
StatusFileSize
new5.09 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,738 pass(es).
[ View ]

i am uploading updated patch as per #29 ( a and b comments ).
Please review.

jhodgdon’s picture

Please make an interdiff file when uploading a new patch with just a couple of changes from the previous patch. Thanks! I'll be happy to review this patch when that is provided, or I might get to it anyway.

visabhishek’s picture

StatusFileSize
new3.43 KB

Thanks jhodgdon,

I will upload interdiff always when I update any patch file. Attaching interdiff for #30, please review.

jhodgdon’s picture

Thanks! I think this patch is good. We should once again run a manual test on the help, with and without the Block module also being enabled:
- Verify that all the links work
- Verify that all mentions of pages/text/permissions within the UI match what is seen in the UI
- Verify that the formatting is OK.

rak2008’s picture

Hi
I looked on the patch and find out on Field help page some messing link for the Datetime
is it okay or not

Thanks

admin/help/field

jhodgdon’s picture

Regarding #34... This issue is about the Menu module help page. So here, we only want the help page for the Menu module to be reviewed (admin/help/menu), and it needs to be reviewed both (a) with the Block module installed and (b) with the Block module uninstalled.

It looks like you have maybe found an issue with a different help page (admin/help/field). In which case, please describe the problem more completely (I couldn't figure out exactly what you were saying the problem was?)... but please put your description of the problem on this other issue rather than here: #2091319: Update hook_help for Field module. Thanks!

rak2008’s picture

Issue tags:-Needs manual testing

Thanks jhodgdon
I verify all the links on page (admin/help/menu, admin/structure/menu, admin/structure/menu/add ) and all work fine.
And I reviewed the Menu with Block installed and I see text viewed on the (admin/structure/menu, admin/structure/menu/add), also i reviewed after uninstalling the Block module and is not showing on the page.
And I verify all mentions on (admin/help/menu) by checking (admin/people/permissions, admin/structure/menu)
For the formatting i see it okay.

jhodgdon’s picture

Component:menu.module» documentation
Status:Needs review» Reviewed & tested by the community

Excellent, thanks for testing!

webchick’s picture

Status:Reviewed & tested by the community» Fixed

Nice work!

Committed and pushed to 8.x. Thanks!

  • Commit 0b9a282 on 8.x by webchick:
    Issue #2090525 by batigolix, ellishettinga, visabhishek, lostkangaroo,...

Status:Fixed» Closed (fixed)

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