Behat integration

Hi mouize,

I did an initial review an opened a couple issues in the project issue queue. Please move this back to "needs review" when those are fixed.

• what is Behat? Please provide a link or an example on the project page.
• git release branch missing, see http://drupal.org/node/1015226
• lines in README.txt should not exceed 80 characters
• hook_install/uninstall don't have doc blocks, please run coder to check your code style http://drupal.org/project/coder
• don't use hook_init()!!!! This hook is executed on literally every page request, even if your module will do nothing there.
• "//check if delta is in behat deltas" Comments should start capitalized and end with a "." There should be a space after "//".
• There are 6 empty lines at the end of the moduel file, one should be enough.
• behat_integration_debug(): empty function, remove it. Use devel for debugging: http://drupal.org/project/devel
• indentation errors in behat_integration_requests.inc, always use 2 spaces.

@mouize has been contacted to ask if the application is abandoned.

After ten weeks with a status of needs work: the applicant may be contacted by a reviewer to determine whether the application was indeed abandoned. The action taken by the reviewer should be documented in the project application issue.

http://drupal.org/node/894256

@mouize replied that the application is not abandoned.

Hello, first off used the online auto scanner. Found many coding style issues. These are included in the attached file. Feel free to run the scanner yourself to resolve these issues.

For Drupal coding conventions please refer to:
http://drupal.org/coding-standards#comment
http://drupal.org/node/1354

Also you need to setup master to just have README. Backing up everything is wise before you do this of course. See:
http://drupal.org/node/1127732

So some more explainations:
With functions like:
bh_project_type_load($type) {
./inc/behat_integration_requests.inc: all functions should be prefixed with your module/theme name to avoid name clashes. See http://drupal.org/node/318#naming [1]
Instead of using "bh" use "behat" which is your module name, and elsewhere on the other functions.

Your .module file needs a @file comment header, check elsewhere also. The above coding standard links will show you the way.

File header on behat.css would be nice as well as the tpl.php files for readability.

That will give you some things to work on for a while. Good luck.

do NOT wrap text in hook_menu functions with t()!

he already moved into a version specific branch..

@darrellulm
what you're doing here is really not very helpfull,
feel free to review applications but please give a little more effort

@patrickd Right, again I didn't know MASTER branch was allowed to have HEAD distro in it before release. It appears that changed again as before MASTER was supposed be empty. Thanks for the update of the change!

Review

/behat_field/behat_field.module
The following function's logic is confusing. You're using three negations where you could use none. Your function returns "not empty" (FALSE) if either the label or value are not empty. Otherwise, if BOTH label and value are empty, your field is "empty" (TRUE).

function behat_field_field_is_empty($item, $field) {
  $has_stuff = FALSE;
  if (!empty($item['label']) || !empty($item['value'])) {
    $has_stuff = TRUE;
  }
  return !$has_stuff;
}

Could be rewritten as (see the logic from my statement above):

function behat_field_field_is_empty($item, $field) {
  return empty($item['label']) && empty($item['value']);
}

Will add some other manual review stuff later, don't want this post to be too long.

• ./behat_field/behat_field.module: all functions should be prefixed with your module/theme name to avoid name clashes. See http://drupal.org/node/318#naming
  

  function behat_field_field_info() {
  function behat_field_field_widget_info() {
  function behat_field_field_widget_form(&$form, &$form_state, $field, $instance, $langcode, $items, $delta, $element) {
  function behat_field_field_is_empty($item, $field) {
  

• ./behat_field/behat_field.install: all functions should be prefixed with your module/theme name to avoid name clashes. See http://drupal.org/node/318#naming
  

  function behat_field_field_schema($field) {
  

• 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. Get a review bonus and we will come back to your application sooner.

FILE: ...es/all/modules/pareview_temp/test_candidate/behat_integration.drush.inc
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
 69 | ERROR | An operator statement must be followed by a single space
--------------------------------------------------------------------------------


FILE: ...l/modules/pareview_temp/test_candidate/inc/behat_integration_entity.inc
--------------------------------------------------------------------------------
FOUND 6 ERROR(S) AFFECTING 6 LINE(S)
--------------------------------------------------------------------------------
  91 | ERROR | Last parameter comment requires a blank newline after it
 168 | ERROR | Doc comment for var $project does not match actual variable name
     |       | $form at position 1
 170 | ERROR | Doc comment for var $op does not match actual variable name
     |       | &$form_state at position 2
 172 | ERROR | Doc comment for var $id does not match actual variable name
     |       | $project at position 3
 218 | ERROR | A unary operator statement must not be followed by a space
 245 | ERROR | Comments may not appear after statements.
--------------------------------------------------------------------------------


FILE: .../modules/pareview_temp/test_candidate/inc/behat_integration_helpers.inc
--------------------------------------------------------------------------------
FOUND 4 ERROR(S) AFFECTING 4 LINE(S)
--------------------------------------------------------------------------------
 252 | ERROR | Last parameter comment requires a blank newline after it
 253 | ERROR | Parameter comment indentation must be 2 additional spaces at
     |       | position 1
 297 | ERROR | Last parameter comment requires a blank newline after it
 333 | ERROR | Last parameter comment requires a blank newline after it
--------------------------------------------------------------------------------


FILE: ...ll/modules/pareview_temp/test_candidate/inc/behat_integration_pages.inc
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
 11 | ERROR | Last parameter comment requires a blank newline after it
 46 | ERROR | A cast statement must be followed by a single space
--------------------------------------------------------------------------------


FILE: ...modules/pareview_temp/test_candidate/inc/behat_integration_projects.inc
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
 138 | ERROR | A unary operator statement must not be followed by a space
--------------------------------------------------------------------------------


FILE: ...modules/pareview_temp/test_candidate/inc/behat_integration_requests.inc
--------------------------------------------------------------------------------
FOUND 12 ERROR(S) AND 2 WARNING(S) AFFECTING 13 LINE(S)
--------------------------------------------------------------------------------
  10 | ERROR   | Last parameter comment requires a blank newline after it
  42 | ERROR   | Doc comment for var $id does not match actual variable name
     |         | $type at position 1
  44 | ERROR   | Last parameter comment requires a blank newline after it
 118 | ERROR   | Last parameter comment requires a blank newline after it
 130 | ERROR   | Last parameter comment requires a blank newline after it
 142 | ERROR   | Last parameter comment requires a blank newline after it
 154 | ERROR   | Last parameter comment requires a blank newline after it
 154 | ERROR   | Doc comment for var $id does not match actual variable name
     |         | $type at position 1
 174 | WARNING | A comma should follow the last multiline array item. Found:
     |         | REQUEST_TIME
 181 | ERROR   | There must be an empty line before the parameter block
 192 | ERROR   | Parameter comment indentation must be 2 additional spaces at
     |         | position 1
 219 | ERROR   | Last parameter comment requires a blank newline after it
 227 | WARNING | A comma should follow the last multiline array item. Found:
     |         | $id
 234 | ERROR   | Doc comment for var $id does not match actual variable name
     |         | $type at position 1
--------------------------------------------------------------------------------


FILE: ...review_temp/test_candidate/sample/features/bootstrap/FeatureContext.php
--------------------------------------------------------------------------------
FOUND 7 ERROR(S) AND 1 WARNING(S) AFFECTING 5 LINE(S)
--------------------------------------------------------------------------------
  2 | ERROR   | Missing file doc comment
 14 | ERROR   | Blank comments are not allowed
 20 | WARNING | Line exceeds 80 characters; contains 103 characters
 20 | ERROR   | No space before comment text; expected "// MinkContext if you
    |         | want to test web" but found "//MinkContext if you want to test
    |         | web"
 20 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or
    |         | question marks
 20 | ERROR   | Comments may not appear after statements.
 21 | ERROR   | Opening brace of a class must be on the same line as the
    |         | definition
 24 | ERROR   | Files must end in a single new line character
--------------------------------------------------------------------------------


FILE: ...pal7/sites/all/modules/pareview_temp/test_candidate/templates/behat.css
--------------------------------------------------------------------------------
FOUND 5 ERROR(S) AFFECTING 3 LINE(S)
--------------------------------------------------------------------------------
  44 | ERROR | Multiple selectors should each be on a single line
  44 | ERROR | Multiple selectors should each be on a single line
  50 | ERROR | Multiple selectors should each be on a single line
  50 | ERROR | Multiple selectors should each be on a single line
 199 | ERROR | Multiple selectors should each be on a single line
--------------------------------------------------------------------------------

Manual review:
1. it would be best prefixing all functions, global variables etc with your project shortname:

if (variable_get('simple_html_dom_path', FALSE)) {
  require_once variable_get('simple_html_dom_path', FALSE);
}
if (variable_get('sfYaml_path', FALSE)) {
  require_once variable_get('sfYaml_path', FALSE);
}
if (variable_get('sfYamlDumper_path', FALSE)) {
  require_once variable_get('sfYamlDumper_path', FALSE);
}
if (variable_get('sfYamlParser_path', FALSE)) {
  require_once variable_get('sfYamlParser_path', FALSE);
}

2. Non initalized variable:

function _behat_integration_tool_test($behat_path = '') {
  if (!$behat_path) {
    $behat_path = variable_get('behat_path', '');
  }
  $exec = $behat_path . ' -V';
  exec($exec, <em><strong>$return</strong></em>);
  if (preg_match('#Behat version#', $return[0])) {
    return TRUE;
  }
  return FALSE;
}

... the same in:
_behat_integration_command()

3. Why do you use redundant functions:

function _behat_integration_type_load($type) {
  return behat_integration_get_project($type);
}

4. allready => already (see behat_integration_project_form_validate)

5. CSS:

#behat .statistics:before {
  content: 'behat';
  position: absolute;
  color: #1c4b20 !important;
  text-shadow: white 1px 1px 1px;
  font-size: 48px !important;
  font-family: Lobster, Tahoma;
  top: 22px;
  left: 20px;
}

should be:

#behat 
.statistics:before {
  content: 'behat';
  position: absolute;
  color: #1c4b20 !important;
  text-shadow: white 1px 1px 1px;
  font-size: 48px !important;
  font-family: Lobster, Tahoma;
  top: 22px;
  left: 20px;
}

Typ:

Get a review bonus and we will come back to your application sooner.

You need 3 reviews for adding the review bonus tag (check the link above).
If you had time, you could take a look at http://drupal.org/node/1302786

Can you address the elements flagged during the manual review in comment #13? (I wasn't sure if you missed the prefixed variable names comment, or had a valid reason for not doing so ... if the latter, then please provide an explanation as to why.)

Apologies if you have addressed them already ... admittedly, I didn't look past the first one.

Please check the coding standard again. You can see the problems at http://ventral.org/pareview/httpgitdrupalorgsandboxmouize1272110git

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