Follow up of #1903750: Convert menu_link hooks to use Type-Hinting to clean up the menu_test.module hook implementations.

Files: 
CommentFileSizeAuthor
#45 fixed-menu-test-comments-1924108-45.patch11.06 KBbenjy
PASSED: [[SimpleTest]]: [MySQL] 57,593 pass(es).
[ View ]
#41 fixed-menu-test-comments-1924108-41.patch11.44 KBbenjy
PASSED: [[SimpleTest]]: [MySQL] 57,955 pass(es).
[ View ]
#39 fixed-menu-test-comments-1924108-39.patch11.29 KBbenjy
PASSED: [[SimpleTest]]: [MySQL] 56,117 pass(es).
[ View ]
#38 fixed-menu-test-comments-1924108-38.patch11.29 KBbenjy
PASSED: [[SimpleTest]]: [MySQL] 57,341 pass(es).
[ View ]
#34 fixed-menu-test-comments-1924108-34.patch10.52 KBbenjy
PASSED: [[SimpleTest]]: [MySQL] 57,854 pass(es).
[ View ]
#31 fixed-menu-test-comments-1924108-31.patch10.53 KBbenjy
PASSED: [[SimpleTest]]: [MySQL] 56,992 pass(es).
[ View ]
#27 fixed-menu-test-comments-1924108-27.patch9.6 KBbenjy
PASSED: [[SimpleTest]]: [MySQL] 55,788 pass(es).
[ View ]
#25 fixed-menu-test-comments-1924108-25.patch6.83 KBbenjy
FAILED: [[SimpleTest]]: [MySQL] 53,670 pass(es), 47 fail(s), and 23 exception(s).
[ View ]
#23 fixed-menu-test-comments-1924108-23.patch6.71 KBbenjy
PASSED: [[SimpleTest]]: [MySQL] 55,975 pass(es).
[ View ]
#20 fixed-menu-test-comments-1924108-20.patch1.74 KBbenjy
PASSED: [[SimpleTest]]: [MySQL] 56,267 pass(es).
[ View ]
#8 fixed-menu-test-comments-1924108-7.patch1.39 KBbenjy
PASSED: [[SimpleTest]]: [MySQL] 56,768 pass(es).
[ View ]
#6 fixed-menu-test-comments-1924108-5.patch1.35 KBbenjy
PASSED: [[SimpleTest]]: [MySQL] 56,456 pass(es).
[ View ]
#1 fixed-menu-test-comments-1924108-1.patch1.31 KBbenjy
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch fixed-menu-test-comments-1924108-1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

benjy’s picture

Status:Active» Needs review
StatusFileSize
new1.31 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch fixed-menu-test-comments-1924108-1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Status:Needs review» Needs work
Issue tags:-@todo clean-up

The last submitted patch, fixed-menu-test-comments-1924108-1.patch, failed testing.

benjy’s picture

Status:Needs work» Needs review
Cottser’s picture

Status:Needs review» Needs work
Issue tags:+@todo clean-up

The last submitted patch, fixed-menu-test-comments-1924108-1.patch, failed testing.

benjy’s picture

Status:Needs work» Needs review
StatusFileSize
new1.35 KB
PASSED: [[SimpleTest]]: [MySQL] 56,456 pass(es).
[ View ]

Re-rolled.

Cottser’s picture

Looks like good clean-up to me, @benjy. While we're here why not touch this up?

+++ b/core/modules/system/tests/modules/menu_test/menu_test.moduleundefined
@@ -582,9 +582,6 @@ function menu_test_theme_callback($argument) {
/**
  * Implement hook_custom_theme().
- *
- * @return
- *   The name of the custom theme to use for the current page.
  */

s/Implement/Implements/

benjy’s picture

StatusFileSize
new1.39 KB
PASSED: [[SimpleTest]]: [MySQL] 56,768 pass(es).
[ View ]

Good spot and fixed.

I can't see it here but where did that "undefined" come from on the end of line one in your previous comment?

Cottser’s picture

benjy’s picture

Dreditor is great. Thanks for the tip!

Cottser’s picture

Status:Needs review» Reviewed & tested by the community

I'd say this is ready to go.

BTW @benjy, it's good practice to link to follow-up issues you create from the original issue. You'll get more issue followers (and reviews) that way too :)

benjy’s picture

OK thanks, will keep that it mind.

alexpott’s picture

Component:system.module» documentation
Assigned:benjy» jhodgdon

Putting this on jhogdon's list

jhodgdon’s picture

Status:Reviewed & tested by the community» Needs review

In the case of a test module (a module meant to be used in tests), there is something to be said for having the documentation for a hook implementation actually explain what the particular implementation does, and how it can be used in a test, rather than just using the standard "Implements hook_foo()" comment.

Any thoughts on that? I think it might be useful to add a line to each function and explain what the test module is doing...

benjy’s picture

@jhodgdon yeah that makes perfect sense to me. I was only doing as instructed by removing them.

I'm happy to create a new patch, removing the @param's but adding in some simple comments to outline the purpose of the test. ?

jhodgdon’s picture

That would be great, thanks!

jhodgdon’s picture

Status:Needs review» Needs work

Given the plan in #15...

benjy’s picture

@jhodgdon, It looks to me as though this patch has been committed?

Either way I think the commenting is already pretty good. For hook_menu_link_insert(), update and delete we only call menu_test_static_variable() which is commented pretty well below.

hook_custom_theme() also has appropriate commenting within the function call. We could move this comment into the doc block if it's used for automatic doc generation?

jhodgdon’s picture

Um, I don't see any evidence that this hook has been committed?

As far as the commenting in the patch being pretty good, that is an OK point that the comments are there in the code and may not need to be in the doc block. I personally think it's a bit better if they're in the doc block, but since it's a test module I guess it is OK... Generally the philosophy of doc blocks is that you should not have to read the code (or the comments in the code) to find out what the function does.

benjy’s picture

StatusFileSize
new1.74 KB
PASSED: [[SimpleTest]]: [MySQL] 56,267 pass(es).
[ View ]

haha, I just jumped back to this and I still had my own patch committed from when I re-rolled it. :)

I've moved the comment into the doc block and a new patch is attached.

jhodgdon’s picture

Assigned:jhodgdon» Unassigned
Status:Needs work» Needs review
jhodgdon’s picture

Title:Remove param and returns from hook implementations» Improve documentation of menu test module
Status:Needs review» Needs work

The patch is fine... I'm looking through the rest of this test module and wondering if it could use additional/better documentation though throughout?

For instance:

a)

/**
* @file
* Dummy module implementing hook menu.
*/

First off, it's not really a "dummy" module. It's a module for testing the menu router system. Second, "hook menu" should be "hook_menu()". Third, it tests a lot of other stuff besides hook_menu(). Can we do better? Or maybe take the description out of the .info.yml file (which is at least more accurate)?

b)

/**
* Implements hook_menu_local_tasks().
*/
function menu_test_menu_local_tasks(&$data, $router_item, $root_path) {
  if (!config('menu_test.settings')->get('tasks.add')) {
    return;
  }

How about adding a line here that describes what this hook implementation does -- something like "If the menu_test.settings configuration 'tasks.add' has been set, adds several local tasks to menu-test/tasks." ? That would help people know what the test module does and how to use it in a test.

c)

/**
* Dummy callback for hook_menu() to point to.
*
* @return
*  A random string.
*/
function menu_test_callback() {
  return 'This is menu_test_callback().';
}

The @return is not accurate here -- it shouldn't say "random", since it isn't random. How about just saying it returns some text that can be tested for? Also, the first line doesn't start with a verb.

d)

/**
* Callback that test menu_test_menu_tree_set_path().
*/
function menu_test_menu_trail_callback() {

Bad grammar (test -> tests).

etc. There are many more similar to b-d in this file.

benjy’s picture

Status:Needs work» Needs review
StatusFileSize
new6.71 KB
PASSED: [[SimpleTest]]: [MySQL] 55,975 pass(es).
[ View ]

New patch fixes mentioned issues and a few others I came across while reading through the module.

jhodgdon’s picture

Status:Needs review» Needs work

Thanks! That's a great start!

A few things to fix up:

a)

-  // @see BreadcrumbTest
+  // @see Drupal\system\Tests\Menu\BreadcrumbTest

If you use namespaces in documentation, they need to start with \ -- this occurs in several places in your patch.

b)

   );
-
+  // Test the access key with a function callback.
   $items['menu_login_callback'] = array(

I wouldn't remove the blank lines here (and in other similar places).

c) There are spaces at the ends of several lines. Configure your text editor to highlight or remove end of line spaces.

d)

/**
- * Callback that test menu_test_menu_tree_set_path().
+ * Callback that tests menu_test_menu_tree_set_path().
  */
function menu_test_menu_trail_callback() {

Function documentation first lines should start with verbs.
http://drupal.org/node/1354#functions

benjy’s picture

StatusFileSize
new6.83 KB
FAILED: [[SimpleTest]]: [MySQL] 53,670 pass(es), 47 fail(s), and 23 exception(s).
[ View ]

New patch attached, not finished however. I wanted your feedback on the documentation I added for menu_test_menu_trail_callback(). I added after reading: https://drupal.org/node/1354#menu-callback

Is this something we should go ahead and do for the other menu callbacks? Maybe we don't want this much info in test modules?

jhodgdon’s picture

We discussed this in IRC. Since the standard for menu callbacks will probably need to be revised for Drupal 8, we probably should not worry about the exact format of the menu callback doc blocks in this module... but we probably won't change the first line standards much. So let's have each of them have a first line like:

Page callback: Displays a list of content.

and documentation of any parameters, as well as the return value (see https://drupal.org/node/1354#menu-callback).

benjy’s picture

Status:Needs work» Needs review
StatusFileSize
new9.6 KB
PASSED: [[SimpleTest]]: [MySQL] 55,788 pass(es).
[ View ]

I've fixed the doc blocks up for menu callbacks and a few other typos.

I also changed a variable name to match the doc block variable name. Not sure what the rule on that is but it makes sense for the parameters to match their doc block names?

-function menu_test_title_callback($title, $case_no = 3) {
-  return t($title) . ' - Case ' . $case_no;
+function menu_test_title_callback($title, $case_number = 3) {
+  return t($title) . ' - Case ' . $case_number;

Status:Needs review» Needs work
Issue tags:-@todo clean-up

The last submitted patch, fixed-menu-test-comments-1924108-27.patch, failed testing.

benjy’s picture

Status:Needs work» Needs review
Issue tags:+@todo clean-up
jhodgdon’s picture

Status:Needs review» Needs work

It's looking pretty close, thanks for the new patch! Yes, @param should match the actual parameters, and in this case the name in @param is better than the one in the function signature, so it is good to change the function rather than the documentation block.

So, I think this still needs a very small amount of work and then we can call it good:

a) menu_test_menu_trail_callback() :

+ * Retrieve the current menu path and if the menu path is not empty updates
+ * the menu path that is used to determine the active menu trail.

The verbs in this sentence do not agree with each other: Retrieve... updates. Should be Retrieves/updates or Retrieve/update (I don't care which).

b) menu_test_theme_page_callback():

  * @param $inherited
  *   An optional boolean to set to TRUE when the requested page is intended to
  *   inherit the theme of its parent.
- * @return
+ * @return string

There should be a blank line before the @return. (this applies elsewhere in the patch too)

Also, if you are adding types to the @return, how about to the @param too (bool) -- in which case we could make it fully compliant with our standards (and more concise):
@param book $inherited
(optional) TRUE if the requested page should inherit the theme of its parent.

benjy’s picture

Status:Needs review» Needs work
StatusFileSize
new10.53 KB
PASSED: [[SimpleTest]]: [MySQL] 56,992 pass(es).
[ View ]

Good catch with the grammer, now fixed.

I've added the new lines after all @return's and types to all @param's to bring the module inline with the new D8 standards. I also added (optional) where appropriate.

benjy’s picture

Status:Needs work» Needs review
jhodgdon’s picture

Priority:Minor» Normal

This looks really good!

One thing: When a param/return can be NULL, we use "null" not "NULL" as the data type:
https://drupal.org/node/1354#types

One other thing: The very first line of the patch...

/**
  * @file
- * Dummy module implementing hook menu.
+ * Test module providing tests for several menu related hooks.
  */

Technically, this module doesn't test anything. It sets up things for the test classes to test. So maybe we should say: "Module that implements various hooks for menu tests." or something like that?

benjy’s picture

Status:Needs work» Needs review
StatusFileSize
new10.52 KB
PASSED: [[SimpleTest]]: [MySQL] 57,854 pass(es).
[ View ]

OK I've fixed the two things you mentioned.

jhodgdon’s picture

Looks good! I'll get this patch committed.

In a follow-up (or if you want to fix it now that would be fine too), we could/should go back and fix up the rest of the function documentation first-line descriptions so that they follow our standards (one line, starting with a verb in 3rd person, with possible "Foo callback:" prefixes, ending in "."). There are still a few that don't, after that patch:
menu_test_argument_load()
menu_test_other_argument_load()
menu_test_menu_name()
menu_test_static_variable()

I also don't think the 2nd paragraph is needed here (redundant):

/**
+ * Title callback: Concatenates the title and case number.
+ *
  * Concatenates a string, by using the t() function and a case number.
...
+ * @return string
+ *   A string containing the title and case number.
+ *
+ * @see menu_test_menu().
  */
function menu_test_title_callback($title, $case_no = 3) {

So... actually, maybe it would be good to fix these before committing the patch?

benjy’s picture

OK, give me a day or so and i'll have a look over the other methods. We may as well get it all in together.

benjy’s picture

Status:Needs review» Needs work
benjy’s picture

Status:Needs work» Needs review
StatusFileSize
new11.29 KB
PASSED: [[SimpleTest]]: [MySQL] 57,341 pass(es).
[ View ]

OK hopefully this should see the menu_test module right :)

benjy’s picture

StatusFileSize
new11.29 KB
PASSED: [[SimpleTest]]: [MySQL] 56,117 pass(es).
[ View ]

Fixed some capitalisation.

jhodgdon’s picture

Status:Needs review» Needs work

Excellent, we're almost there! Just a couple of very minor things to fix:

a) In @param/@return data types, we say "false" and "null" (this is to keep all of them lower-case, as they are on php.net and elsewhere), but in text, we use FALSE/NULL. Sorry for the confusion. So:

+ * @return false
+ *   Always return false.

should be

+ * @return false
+ *   Always returns FALSE.

b)

/**
- * Helper function for the testMenuName() test. Used to change the menu_name
- * parameter of a menu.
+ * Sets a static variable for the testMenuName() test.
+ * Used to change the menu_name parameter of a menu.

Needs a blank line between the first two lines.

benjy’s picture

Status:Needs work» Needs review
StatusFileSize
new11.44 KB
PASSED: [[SimpleTest]]: [MySQL] 57,955 pass(es).
[ View ]

OK fixed up the references to false in the text, added the blank line and in the menu_test_menu_name() function I changed the $name variable to be $menu_name the same as the docs.

jhodgdon’s picture

Assigned:Unassigned» jhodgdon
Status:Needs review» Reviewed & tested by the community

An interdiff would have been nice. :)

No worries though -- this looks good. Thanks! I'll get it committed shortly.

jhodgdon’s picture

Status:Reviewed & tested by the community» Needs work

The patch unfortunately does not apply today. :(

jhodgdon’s picture

Assigned:jhodgdon» Unassigned
benjy’s picture

Status:Needs work» Needs review
StatusFileSize
new11.06 KB
PASSED: [[SimpleTest]]: [MySQL] 57,593 pass(es).
[ View ]

OK it looks like hook_menu_site_status_alter() was removed in 1998228.

Patch rerolled.

When is it courtesy to post an interdiff? Every time you change a patch?

jhodgdon’s picture

Regarding interdiffs, if the patch is more than a few lines long, and you make changes, it is nice to include an interdiff for reviewers. For a reroll, this is not really possible/necessary.

The last submitted patch, fixed-menu-test-comments-1924108-45.patch, failed testing.

benjy’s picture

Status:Needs work» Needs review
Issue tags:+@todo clean-up
jhodgdon’s picture

Assigned:Unassigned» jhodgdon
Status:Needs review» Reviewed & tested by the community

Thanks! Looks good still/again. I'll get it committed soon.

jhodgdon’s picture

Assigned:jhodgdon» Unassigned
Status:Reviewed & tested by the community» Fixed
Issue tags:-@todo clean-up

Thanks again! Committed to 8.x.

Status:Fixed» Closed (fixed)

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