Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Follow up of #1903750: Convert menu_link hooks to use Type-Hinting to clean up the menu_test.module hook implementations.
Comment | File | Size | Author |
---|---|---|---|
#45 | fixed-menu-test-comments-1924108-45.patch | 11.06 KB | benjy |
#41 | fixed-menu-test-comments-1924108-41.patch | 11.44 KB | benjy |
#39 | fixed-menu-test-comments-1924108-39.patch | 11.29 KB | benjy |
#38 | fixed-menu-test-comments-1924108-38.patch | 11.29 KB | benjy |
#34 | fixed-menu-test-comments-1924108-34.patch | 10.52 KB | benjy |
Comments
Comment #1
benjy CreditAttribution: benjy commentedComment #3
benjy CreditAttribution: benjy commented#1: fixed-menu-test-comments-1924108-1.patch queued for re-testing.
Comment #4
star-szr#1: fixed-menu-test-comments-1924108-1.patch queued for re-testing.
Comment #6
benjy CreditAttribution: benjy commentedRe-rolled.
Comment #7
star-szrLooks like good clean-up to me, @benjy. While we're here why not touch this up?
s/Implement/Implements/
Comment #8
benjy CreditAttribution: benjy commentedGood 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?
Comment #9
star-szrJust a Dreditor bug: #1754036: "undefined" suffix.
Comment #10
benjy CreditAttribution: benjy commentedDreditor is great. Thanks for the tip!
Comment #11
star-szrI'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 :)
Comment #12
benjy CreditAttribution: benjy commentedOK thanks, will keep that it mind.
Comment #13
alexpottPutting this on jhogdon's list
Comment #14
jhodgdonIn 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...
Comment #15
benjy CreditAttribution: benjy commented@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. ?
Comment #16
jhodgdonThat would be great, thanks!
Comment #17
jhodgdonGiven the plan in #15...
Comment #18
benjy CreditAttribution: benjy commented@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?
Comment #19
jhodgdonUm, 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.
Comment #20
benjy CreditAttribution: benjy commentedhaha, 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.
Comment #21
jhodgdonComment #22
jhodgdonThe 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)
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)
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)
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)
Bad grammar (test -> tests).
etc. There are many more similar to b-d in this file.
Comment #23
benjy CreditAttribution: benjy commentedNew patch fixes mentioned issues and a few others I came across while reading through the module.
Comment #24
jhodgdonThanks! That's a great start!
A few things to fix up:
a)
If you use namespaces in documentation, they need to start with \ -- this occurs in several places in your patch.
b)
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)
Function documentation first lines should start with verbs.
http://drupal.org/node/1354#functions
Comment #25
benjy CreditAttribution: benjy commentedNew 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?
Comment #26
jhodgdonWe 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).
Comment #27
benjy CreditAttribution: benjy commentedI'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?
Comment #29
benjy CreditAttribution: benjy commented#27: fixed-menu-test-comments-1924108-27.patch queued for re-testing.
Comment #30
jhodgdonIt'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() :
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():
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.
Comment #31
benjy CreditAttribution: benjy commentedGood 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.
Comment #32
benjy CreditAttribution: benjy commentedComment #33
jhodgdonThis 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...
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?
Comment #34
benjy CreditAttribution: benjy commentedOK I've fixed the two things you mentioned.
Comment #35
jhodgdonLooks 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):
So... actually, maybe it would be good to fix these before committing the patch?
Comment #36
benjy CreditAttribution: benjy commentedOK, 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.
Comment #37
benjy CreditAttribution: benjy commentedComment #38
benjy CreditAttribution: benjy commentedOK hopefully this should see the menu_test module right :)
Comment #39
benjy CreditAttribution: benjy commentedFixed some capitalisation.
Comment #40
jhodgdonExcellent, 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:
should be
b)
Needs a blank line between the first two lines.
Comment #41
benjy CreditAttribution: benjy commentedOK 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.
Comment #42
jhodgdonAn interdiff would have been nice. :)
No worries though -- this looks good. Thanks! I'll get it committed shortly.
Comment #43
jhodgdonThe patch unfortunately does not apply today. :(
Comment #44
jhodgdonComment #45
benjy CreditAttribution: benjy commentedOK 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?
Comment #46
jhodgdonRegarding 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.
Comment #48
benjy CreditAttribution: benjy commented#45: fixed-menu-test-comments-1924108-45.patch queued for re-testing.
Comment #49
jhodgdonThanks! Looks good still/again. I'll get it committed soon.
Comment #50
jhodgdonThanks again! Committed to 8.x.