In core/modules/system/tests/modules/ajax_forms_test/ajax_forms_test.module there are two docblocks with an @return directive of unknown_type. To be consistent with the rest of core, it should be mixed. Patch to follow.

Files: 
CommentFileSizeAuthor
#10 system-return-values-1754712-9.patch685 bytesTravisCarden
PASSED: [[SimpleTest]]: [MySQL] 39,386 pass(es). View
#5 system-return-values-1754712-5.patch785 bytespingers
PASSED: [[SimpleTest]]: [MySQL] 40,709 pass(es). View
#2 system-return-values-1754712-2.patch796 bytesBußmeyer
PASSED: [[SimpleTest]]: [MySQL] 40,648 pass(es). View
#1 system-return-mixed-1754712-1.patch814 bytesTravisCarden
PASSED: [[SimpleTest]]: [MySQL] 40,634 pass(es). View

Comments

TravisCarden’s picture

Status: Active » Needs review
Issue tags: +Novice
FileSize
814 bytes
PASSED: [[SimpleTest]]: [MySQL] 40,634 pass(es). View
Bußmeyer’s picture

FileSize
796 bytes
PASSED: [[SimpleTest]]: [MySQL] 40,648 pass(es). View

Hey TravisCarden,
Thank you for your patch. I have reviewed them.

function ajax_forms_test_menu() {
is an implemetation of a hook. The params and return values should not be included. See: http://drupal.org/node/1354#hookimpl

/**
 * Form to display the Ajax Commands.
 * @param $form
 * @param $form_state
 * @return undefined
 */
function ajax_forms_test_ajax_commands_form($form, &$form_state) {

This function defines a form, right? The return value is an array isn't it?

I created a new patch.

Regards,
Thomas

mcjim’s picture

Status: Needs review » Needs work

Thanks both for your patches.
Bußmeyer is right, implementations of a hook don't include the params and return values.

Form generating functions don't, either (well, not for $form and $form_state or the return value), so we can have just:

/**
 * Form to display the Ajax Commands.
 */
function ajax_forms_test_ajax_commands_form($form, &$form_state) {

According to http://drupal.org/node/1354#forms, you can also add @ingroup forms, which groups all forms into the Form Builder topic on api.drupal.org.

So… almost there :-)

TravisCarden’s picture

Thank you both. @mcjim: This is just a test module. Do we @ingroup forms functions in tests?

pingers’s picture

FileSize
785 bytes
PASSED: [[SimpleTest]]: [MySQL] 40,709 pass(es). View

From /core/modules

grep -r ingroup . | grep test
./simpletest/simpletest.pages.inc: * @ingroup themeable
./simpletest/simpletest.pages.inc: * @ingroup themeable
./simpletest/simpletest.pages.inc: * @ingroup forms
./system/tests/modules/form_test/form_test.module: * @ingroup forms
./system/tests/modules/form_test/form_test.module: * @ingroup forms
./system/tests/modules/form_test/form_test.module: * @ingroup forms
./system/tests/modules/form_test/form_test.module: * @ingroup forms
./system/tests/modules/form_test/form_test.module: * @ingroup forms
./file/tests/file_module_test.module: * @ingroup forms

Seems a bit inconsistent re: @ingroup. Re-rolled without the change.

pingers’s picture

Status: Needs work » Needs review

Doh. Test it!

mcjim’s picture

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

Interesting observation about use of @ingroup, pingers.
Looking at core/modules/system/tests/form_test, the term "Form constructor" is used 10 times but "@ingroup forms" only 5. But that's an inconsistency to be discussed in another issue.

I think this is good to go.

webchick’s picture

Component: system.module » documentation
Status: Reviewed & tested by the community » Fixed

This looks right to me!

Committed and pushed to 8.x. Thanks!

Does this need backport to D7 as well?

jhodgdon’s picture

Version: 8.x-dev » 7.x-dev
Status: Fixed » Patch (to be ported)

Looks like it does need a backport. The file is
modules/simpletest/tests/ajax_forms_test.module

TravisCarden’s picture

Status: Patch (to be ported) » Needs review
FileSize
685 bytes
PASSED: [[SimpleTest]]: [MySQL] 39,386 pass(es). View
TravisCarden’s picture

Status: Needs review » Reviewed & tested by the community

A straight backport of a trivial patch can be RTBC'ed once it passes automated testing, no?

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Looks the same to me. :)

Committed and pushed to 7.x. Thanks!

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