Buttonator

I have not reviewed your project but I did run it through pareview.sh
Output below
--------------------------------------------

It appears you are working in the "master" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.
Review of the master branch:

• README.txt is missing, see the guidelines for in-project documentation.
• Remove "version" from the info file, it will be added by drupal.org packaging automatically.
• Drupal Code Sniffer has found some code style issues (please check the Drupal coding standards). See attachment.

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. Go and review some other project applications, so we can get back to yours sooner.

FILE: ...es/all/modules/pareview_temp/test_candidate/buttonator.admin.delete.inc
--------------------------------------------------------------------------------
FOUND 5 ERROR(S) AFFECTING 5 LINE(S)
--------------------------------------------------------------------------------
 21 | ERROR | No space before comment text; expected "// Check to make sure
    |       | this ID exists." but found "//Check to make sure this ID exists."
 24 | ERROR | Line indented incorrectly; expected 2 spaces, found 4
 29 | ERROR | No space before comment text; expected "// Build the form." but
    |       | found "//Build the form."
 62 | ERROR | Line indented incorrectly; expected 2 spaces, found 4
 73 | ERROR | Line indented incorrectly; expected 2 spaces, found 4
--------------------------------------------------------------------------------


FILE: ...ites/all/modules/pareview_temp/test_candidate/buttonator.admin.edit.inc
--------------------------------------------------------------------------------
FOUND 7 ERROR(S) AFFECTING 7 LINE(S)
--------------------------------------------------------------------------------
  21 | ERROR | No space before comment text; expected "// Fetch the override
     |       | from the database." but found "//Fetch the override from the
     |       | database."
  24 | ERROR | Line indented incorrectly; expected 2 spaces, found 4
  29 | ERROR | No space before comment text; expected "// Build the form." but
     |       | found "//Build the form."
  71 | ERROR | Line indented incorrectly; expected 2 spaces, found 4
  75 | ERROR | Line indented incorrectly; expected 2 spaces, found 4
  79 | ERROR | Line indented incorrectly; expected 2 spaces, found 4
 102 | ERROR | Files must end in a single new line character
--------------------------------------------------------------------------------


FILE: ...iew/sites/all/modules/pareview_temp/test_candidate/buttonator.admin.inc
--------------------------------------------------------------------------------
FOUND 6 ERROR(S) AND 2 WARNING(S) AFFECTING 8 LINE(S)
--------------------------------------------------------------------------------
  24 | ERROR   | Line indented incorrectly; expected 2 spaces, found 4
  69 | ERROR   | Line indented incorrectly; expected 4 spaces, found 8
 112 | ERROR   | Line indented incorrectly; expected 2 spaces, found 4
 131 | WARNING | Avoid backslash escaping in translatable strings when
     |         | possible, use "" quotes instead
 137 | WARNING | Avoid backslash escaping in translatable strings when
     |         | possible, use "" quotes instead
 159 | ERROR   | Line indented incorrectly; expected 2 spaces, found 4
 163 | ERROR   | Line indented incorrectly; expected 2 spaces, found 4
 167 | ERROR   | Line indented incorrectly; expected 2 spaces, found 4
--------------------------------------------------------------------------------


FILE: ...-pareview/sites/all/modules/pareview_temp/test_candidate/buttonator.inc
--------------------------------------------------------------------------------
FOUND 24 ERROR(S) AFFECTING 24 LINE(S)
--------------------------------------------------------------------------------
  10 | ERROR | Data type of return value is missing
  11 | ERROR | Return comment indentation must be 2 additional spaces
  20 | ERROR | Missing parameter type at position 1
  21 | ERROR | Parameter comment indentation must be 2 additional spaces at
     |       | position 1
  23 | ERROR | Data type of return value is missing
  24 | ERROR | Return comment indentation must be 2 additional spaces
  30 | ERROR | Line indented incorrectly; expected 2 spaces, found 4
  37 | ERROR | Line indented incorrectly; expected 2 spaces, found 4
  49 | ERROR | Line indented incorrectly; expected 2 spaces, found 4
  57 | ERROR | Missing parameter type at position 1
  58 | ERROR | Parameter comment indentation must be 2 additional spaces at
     |       | position 1
  60 | ERROR | Data type of return value is missing
  61 | ERROR | Return comment indentation must be 2 additional spaces
  81 | ERROR | Missing parameter type at position 1
  82 | ERROR | Parameter comment indentation must be 2 additional spaces at
     |       | position 1
  84 | ERROR | Missing parameter type at position 2
  85 | ERROR | Parameter comment indentation must be 2 additional spaces at
     |       | position 2
  87 | ERROR | Missing parameter type at position 3
  88 | ERROR | Parameter comment indentation must be 2 additional spaces at
     |       | position 3
  91 | ERROR | Line indented incorrectly; expected 2 spaces, found 4
  92 | ERROR | Line indented incorrectly; expected 4 spaces, found 8
  93 | ERROR | Line indented incorrectly; expected 6 spaces, found 12
  99 | ERROR | Line indented incorrectly; expected 4 spaces, found 8
 109 | ERROR | Files must end in a single new line character
--------------------------------------------------------------------------------


FILE: ...eview/sites/all/modules/pareview_temp/test_candidate/buttonator.install
--------------------------------------------------------------------------------
FOUND 3 ERROR(S) AFFECTING 3 LINE(S)
--------------------------------------------------------------------------------
 15 | ERROR | Function doc comment must end on the line before the function
    |       | definition
 55 | ERROR | You must use "/**" style comments for a function comment
 58 | ERROR | Files must end in a single new line character
--------------------------------------------------------------------------------


FILE: ...review/sites/all/modules/pareview_temp/test_candidate/buttonator.module
--------------------------------------------------------------------------------
FOUND 24 ERROR(S) AFFECTING 23 LINE(S)
--------------------------------------------------------------------------------
  83 | ERROR | No space before comment text; expected "// Get the list of
     |       | overrides and perform the overrides if necessary." but found
     |       | "//Get the list of overrides and perform the overrides if
     |       | necessary."
  85 | ERROR | Line indented incorrectly; expected 2 spaces, found 4
  86 | ERROR | Line indented incorrectly; expected 4 spaces, found 8
  91 | ERROR | No space before comment text; expected "// Print the form IDs if
     |       | necessary." but found "//Print the form IDs if necessary."
  93 | ERROR | Line indented incorrectly; expected 2 spaces, found 4
 102 | ERROR | Return comment must be on the next line
 105 | ERROR | "include_once" is a statement not a function; no parentheses are
     |       | required
 112 | ERROR | Missing parameter type at position 1
 113 | ERROR | Parameter comment indentation must be 2 additional spaces at
     |       | position 1
 115 | ERROR | Return comment must be on the next line
 118 | ERROR | "include_once" is a statement not a function; no parentheses are
     |       | required
 125 | ERROR | Doc comment for var $form_id does not match actual variable name
     |       | $id at position 1
 125 | ERROR | Missing parameter type at position 1
 126 | ERROR | Parameter comment indentation must be 2 additional spaces at
     |       | position 1
 128 | ERROR | Return comment must be on the next line
 131 | ERROR | "include_once" is a statement not a function; no parentheses are
     |       | required
 139 | ERROR | Missing parameter type at position 1
 140 | ERROR | Parameter comment indentation must be 2 additional spaces at
     |       | position 1
 142 | ERROR | Missing parameter type at position 2
 143 | ERROR | Parameter comment indentation must be 2 additional spaces at
     |       | position 2
 145 | ERROR | Missing parameter type at position 3
 146 | ERROR | Parameter comment indentation must be 2 additional spaces at
     |       | position 3
 149 | ERROR | "include_once" is a statement not a function; no parentheses are
     |       | required
 154 | ERROR | Files must end in a single new line character
--------------------------------------------------------------------------------

Source: http://ventral.org/pareview - PAReview.sh online service

It does not seem to work for me. Please let me know what I am doing wrong.

Attached screenshots.

Also please provide the updated git repository in the top thread.

Alright I will try it this evening.

Hi doana,

I downloaded and tested out your module on a localhost D7 install. Very cool idea, and it works very well. Worked like a charm on my page node creation form and contact form. It also enabled/disabled/uninstalled just fine via Drush.

I ran your code through PAReview: http://ventral.org/pareview/httpgitdrupalorgsandboxdoana1392138git

This is what it had to say:

It appears you are working in the "master" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.

This is because your D7 branch name is '7.x-1.x-dev' and it should be named '7.x-1.x'.

The only thing I recommend is using module_load_include('inc','buttonator','buttonator') instead of the require_once statements in your module file. Other than that, looks good to me.

Updated git checkout line to match the new dev branch.

Hi Adam,

I gave your module another try on a fresh Drupal 7.12 install on localhost. I tried it out on the following forms:

user_login
user_pass
user_register_form
page_node_form
comment_node_page_form

Everything worked great, just like last time. I manually reviewed each of your files:

buttonator.admin.delete.inc
buttonator.admin.edit.inc
buttonator.admin.inc
buttonator.inc
buttonator.info
buttonator.install
buttonator.module

I see good use of comments and proper database API usage. Additionally, I now see the use of module_load_include('inc','buttonator','buttonator') instead of require_once.

I also tried editing and deleting existing buttonator overrides, and everything worked as expected. Again, the module installed just fine via drush, and uninstalled cleanly. I checked to make sure this was true the old fashioned way (clicking enable/disable/uninstall) on the module administration page, all worked as expected there too.

Nice job cleaning everything up, the Pareview comes back clean (http://ventral.org/pareview/httpgitdrupalorgsandboxdoana1392138git).

In my opinion this is ready for a release, happy coding!

Review of the 7.x-1.x branch:

• Drupal Code Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. Get a review bonus and we will come back to your application sooner.

manual review:

• "'access arguments' => array('administer content types'),": this permission does not really fit, create your own.
• buttonator_form_alter(): indentation error in the if() statement, always use 2 spaces per level.
• buttonator_form_alter(): why don't you pass the form id to buttonator_get_overrides_by_form()?
• buttonator_get_overrides_by_form(): why do you need that function? You could just specify an optional from id parameter in buttonator_get_overrides(). Then you could also remove _buttonator_get_overrides() and rename _buttonator_get_overrides_by_id().
• _buttonator_override(): you should use element_children() here to do the recursion not on # elements.
• _buttonator_edit(): if you are just returning a form you should use drupal_get_form as page callback in hook_menu() and remove that function.
• _buttonator_edit_form(): the form id is passed as third parameter to that function, so you should use it instead of $form_state['build_info']['args'][0]. Even better: use the auto loading mechanism in hook_menu and you will get a pre-populated object here. You won't need the error message then if it does not exist.
• If a form has a submit function, then hidden form values are not needed. Instead, any values that you need to pass to $form_state['values'] can be declared in the $form array as '#type' => 'value'.
• 'Please enter a form ID.': all user facing text must run through t() for translation.
• "$edit_link = '<a href="' . $edit_src . '">Edit</a>';": do not create link markup yourself, use l() instead.
• do not build html tables yourself, use theme('table', ...) instead. see also http://api.drupal.org/api/drupal/includes!theme.inc/function/theme_table/7
• _buttonator_admin_settings_validate(): empty function, so remove it.
• _buttonator_admin_settings_submit(): remove that function and use system_settings_form() in _buttonator_admin_settings() instead.

Hi,

Here's a manual review from me. I installed the module, and came across the following :

1. Would it be an idea to uppercase the 'b' in 'buttonator' in your .info file? Module names in Drupal usually do this.
2. When adding a new override, the description for the 'Action' field says

Enter the name of an button to override. For example to override the text of the Save button enter 'Save'

I'm a little in doubt about whether you are talking about the button name, or the button text? The button name is usually something like 'submit', while the button text might be something else.

3. Will your module be able to handle multiple languages?

Last, how does this module differ from String Overrides?

Switching to needs work because of comment #13.

Closing due to lack of activity. Feel free to reopen if you are still working on this application.

Updated git clone command with the proper branch (7.x-1.x)