Speed Form Blocks

http://ventral.org/pareview/httpgitdrupalorgsandboxpgogy1370108git-7dev
Please tell me which ones you don't understand, I'll try to explain them to you.

Your not working correctly with branches to create project releases. 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.

  }
  else{

->

  }
  else {

my editor is also showing this, maybe you should switch your editor

Hi,

Quite big module :-)
Here is my review of the 7.x-1.1.dev branch. Should be 7.x-1.x-dev.

Master Branch

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.

Readme

Please take a moment to make your README.txt follow the guidelines for in-project documentation.

Filename should be README.txt

speed_form_blocks.block_creation.zip

You have a zip in the module directory, what it is for?

speed_forms_block.info

Remove configure = admin/structure/speed_form_blocks, more about structure of the info-file here: http://drupal.org/node/171205

Remove@ author and @version

That information could instead be in the README.txt

Santanize POST

In must places you sanitize values - but in speed_form_blocks_management_handler you do that first after you add the value to the foreach - why?

Also you have some formatting issues, mostly about identing: http://ventral.org/pareview/httpgitdrupalorgsandboxpgogy1370108git-7x-11dev

When I enable the module (with drush) I get the warning

Invalid argument supplied for foreach() block.module:385                      [warning]

Also when I try to create a new block with the name Testblock I get the warning

The machine-readable name must contain only lowercase letters, numbers, and underscores.

But it does not say in the description that the name need to follow that pattern

In the field 'Name for this form element', the does not disappear when it is focused, I have to manually delete the text in the field, the same for User guidance for this field

I create a block with a textfield. Add it to a sidebar. The textfield shows, but no text on the submit button. I can submit. I do not get any message. After that I notice that you can add text to the button and other stuff also. When I do so, it works. Maybe I should have known that if there were text in the README :-)

I think you only have some small issues to take care about. It works, but need some small improvements.

1. This is what I get when I install (using Drush):

The following extensions will be enabled: speed_form_blocks
Do you really want to continue? (y/n): y
Invalid argument supplied for foreach() block.module:385             [warning]
Invalid argument supplied for foreach() block.module:385             [warning]
speed_form_blocks was enabled successfully.                          [ok]

Installation errors = bad.

2. When I 'start' a block and add 1 field, I get this form:

That's very unclear. The element I just made, now appears twice and the form has grown bigger... What do I do next? Since the page has refreshed (and changed) a message would be very useful.

3. 'file path' => $path, in hook_menu is unnecessary, since $path is your module path and that's the default.

4. I'm seeing a lot of trailing spaces. I'm not sure how the Drupal coding standards like those, but I don't. Your editor probably has a macro to remove those. (In (Win) Notepad++ it's Alt+Shift+S.)

5. This isn't the right comment format:

/**
   *
   * Function speed_form_blocks_forms
   * This function handles the drupal hook to have one function to display the forms created using this module
   * @version 1.0
   * @author Patrick Lockley
   */

Use:

/**
 * Implements hook_forms().
 *
 * This function handles the drupal hook to have one function to display the forms created using this module. << Note the trailing period.
 */

6. Pareview finds a lot of issues.

I'm on branch 7.x-1.x-dev btw. (I think "7.x-1.x" would be good enough. A branch is always dev. A release needs that flag, a branch doesn't.)

Your function speed_form_blocks_deletion_handler is really bad/wrong/incorrect. You should never, ever have to call $_POST in a submit handler. (Do correct me if I'm wrong.)

Never, ever, ever trust user input.

The Form API filters use input for you. Use $form_state['values'] to get it.

1. Function comments are still a bit off. Write them to be parsed by a docs generator. Once your module becomes hugely popular, it will be useful for developers. See this nodequeue example. Extracted from this comment.

Specifically: no need to mention the function's name, version or author. Version and author are okay, if you want, but the name is unnecessarily verbose.

2. Pareview tells me: README.txt is missing, see the guidelines for in-project documentation.

3. The form is more obvious now, but it's still imperfect IMO. If you want to add 4 elements, the form will be huge in the end. You could (?) save the block ('unpublished') between creating elements.

4. I checked out branch 7.x-1.x, but I still see $_POST in speed_form_blocks_callback(). Am I behind or is it actually still there?

5. Any reason you're explicitly not caching in speed_form_blocks_block_info()?

6. I'm surprised Pareview doesn't mention this:

          case 0 : $type = 'checkbox';
            break;
          case 1 : $type = 'date';
            break;
          case 2 : $type = 'textformat';
            break;

See the coding standards on switch (...).

7. On admin/structure/speed_form_blocks/creation, you could use type machine_name to fill the machine name from a title. (Ignore this if you think there's reason for a block to have a human readable title.) See the Form API for examples and details on machine_name.

8. This is tiny and in no way related to Drupal's coding standards, but it's related to mine =) This is (very) inefficient:

      for ($x = 0; $x < count($form_state['storage']['form_elements']); $x++) {

because you're counting every iteration of the loop, but the count doesn't change. Count it once and use that number in the loop. Also good for readability:

      $size = count($form_state['storage']['form_elements']);
      for ($x = 0; $x < $size; $x++) {

Like I said: tiny.

9. You might want to consider requiring fields in the creation form. Everything except "User prompt ..." I'd say. Crazy results if you add a few nameless fields =)

Beyond these few thingies, I like.

Two more things

10. The "Button label" and "CSS class" (block properties) are textarea's... Is that intentional? Textfields feel more natural and make more sense IMO.

11. More fields in the Block management tab should be required. I'd say all of them except "HTML before", "HTML after" and "CSS classes".

Okay, just two more.

12. No way to edit a block?

13. Are all the tabs necessary? There's a lof of them.

• You could fit admin/structure/speed_form_blocks into a hook_help.
• I think you could combine Block management and Deletion (maybe)
• You can definitely combine Manage feedback and Responses

Oh btw... The Webform module offers an option to render webforms as blocks. Very nicely actually. Optionally including the rendered node.

You might want to consider that =) Your time might be spent more wisely contributing to Webform.

And that's four in a row. That's enough. Cool module!

The README is unacceptable =)

This is from the project page. Much better than the module's README.

Designed to allow the simple and fast creation of forms as part of a block.

Built to solve a problem with requiring simple forms for user feedback on a site and not having an obvious module to do so.

• Which modules does it include? (One project has >= 1 module.)
• What does it do?
• What happens when I turn it on? (Where are the settings?)
• Do I need to be careful of something? Any tips on usage?
• I am a module. Can I extend you? Where? How? Hooks? Interface?
• Any modules that you're friendly with? Extensions? Plugins?

It's good to have this in the README as well as on the project page.

Sorry man. I'm pretty new to moduling and reviewing myself, so I don't want to be too easy.

Write docs and README's with this in mind:

• everybody is stupid and needs explanation
• nobody reads

(Not developer docs!)

Two more, but maybe you should've learned these by trying/yourself:

1. You can write

function speed_form_blocks_forms($form_id, $args) {

like

function speed_form_blocks_forms($form, $form_state, $delta = NULL) {

and not do the $args and build_info thing. I guess the drupal_get_form docs aren't very forthcoming about that... Well there you have it! Arguments passed to drupal_get_form are passed to the form definition function directly. Just in case though, make them optional.

2. Drupal 7 has a type => 'action' form element. Usually that's where forms put their actions/(submit) buttons. (You can add $data->post_html to that element like you're doing now, OR create a separate (type => 'markup') element for that.)

AUCH:

      '#title' => t($block->block_name . " Other settings (click to minimise)"),

I almost missed that one. That is very uncool. You know why. Fix that. Only string literals in t()!

#type => actions is better. Every form's (submit) buttons should be in an actions container. Do note that it is "actions" and not "action". If you forget about anything Form API, just search for "drupal fapi" anywhere.

A few more pointers you can use so you wish:

See the configure directive on Writing .info files. For users it's immensely useful to know where to start (configuring).

The very most important and powerful hook in Drupal is the form_alter. Every developer starts there if they want to change something. Make sure your forms return something alterable. Currently the element names aren't optimal:

    [test_So... What's going on here?] => Array
        (
            [#type] => textfield
            [#title] => So... What's going on here?
        )

That's because the "Name for this form element" elements can contain anything. (Including nothing.) See attached screenshot. Most of those form elements should be #required. (Definitely Name and User prompt.)

I usually write queries like this: (note the extra spaces on lines 2 & 3)

    $num_deleted = db_delete('speed_form_blocks_created')
      ->condition('bid', str_replace("delete_", "", $key))
      ->execute();

Why all the white space here:

    '#markup'          => '<p>Deleting a block will not delete data</p>',

Why in every function:

  global $base_url;

This is problably not right: (double check_plain)

    $checked = check_plain($value_to_check);

    drupal_set_message(check_plain($checked) . t(": This Block name is already in use. Please choose another name"), 'warning');

Also the t() string is strange. Don't start with ": ". Better use a placeholder inside the translatable string: t('@block_name: This block .....'). (This happens a few times.)

White space:

    $form[$block->block_name][$block->block_name . "_blockname"] = array(
      '#type'          => 'hidden',
      '#value' => $block->block_name,
    );

Why the check_plain?

        db_update('speed_form_blocks_created')
        ->condition('bid', $value[$key . "_bid"])
        ->fields(array('block_name' => check_plain($value[$key . "_newname"])))
        ->execute();

There's a few more things I would've done differently, but different isn't necessarily better and definitely personal.

There's one must fix. After that I think I'm satisfied =)

Almost =)

1.

    drupal_set_message(check_plain($value_to_check) . t(": This Block name is already in use. Please choose another name- FUCK OFF"), 'warning');

t() string is still off. Don't start with ": ". Include the param in the t() string or add ": " in between. And obviously there's one "FUCK OFF" too many.

Also: if this message is a blocker, show it as an "error", not a "warning". Block names MUST be unique, so a duplicate is a validation error. Warnings aren't blockers (the block would be saved, but not perfectly).

Perfect IMO:

    drupal_set_message(t("Block name %name is already in use. Please choose another name", array('%name' => $value_to_check)), 'error');

Didn't know %? There's three types of placeholder.

2.

    '#markup' => t('<h3>Add a new field, or edit the field above</h3>'),

Better to have the tags outside the t() string, so you can change markup later without changing the t() string. The same in ...response.inc.

    '#markup' => '<h3>' . t('Add a new field, or edit the field above') . '</h3>',

3.

    $form['inline_feedback_main'] = array(
      '#markup'          => '<h3>Responses received from Speed form blocks</h3>',
    );

Should be translated. In a few more locations. Even standalone words like "Settings" must be translated. Check every English literal you type. All of them must be translated in the end. (Some directly (like here) and some indirectly (like menu item titles).)

4.

    'title' => t('Speed Form Blocks'),
    'description' => t('Create Speed form blocks'),

Menu item titles shouldn't be translated. The menu system does that (via "title callback"). See hook_menu. The same goes for "description".

5.

It's not wrong per se, but I'm not a fan. In fact, it's a horrible practice:

    foreach ($result as $result) {
      // ...
      return TRUE;
    }
    return FALSE;

Don't foreach when you're bowing out at the first element. Just fetch the first record and check against it:

    $query = db_query('... LIMIT 1'); // or use the query builder: db_select(..)->...
    $exists = $query->fetch();
    return (bool)$exists;

And a minor (IMO) but simple: http://ventral.org/pareview/httpgitdrupalorgsandboxpgogy1370108git Only a README in master branch.

Excellent =) Let's get it out there.

The actual approving has to be done by someone else. I think only a few can. Now it's "Reviewed and tested by the community", that shouldn't take long.

If you need more feedback, you can join several Drupal Groups, one of which: http://groups.drupal.org/code-review

Would you like to take part in the review bonus program? I personally only review/approve applications with a review bonus, but of course you can also wait for other git administrators to take a look at your code.

Don't you trust me, @klausi? =)

this is not about trust, a git admin should do a (hopefully) last review before fixing an issue. ;)

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:

• project page is too short, see http://drupal.org/node/997024
• commit messages: "Signed-off-by: unknown ": this is not very help full, please see http://drupal.org//node/52287
• speed_form_blocks_schema(): doc block is wrong, see http://drupal.org/node/1354#hookimpl
• speed_form_blocks_schema(): why is the block column 2000 in length? I think block names are a lot shorter?
• speed_form_blocks_schema(): the extra field should use the "serialize" property, see http://drupal.org/node/146939 . Also you should probably use a different type (blob?), see http://drupal.org/node/159605
• speed_form_blocks_schema(): please also check your other db columns, you use varchar and 2000 where it might not make sense.
• "@version 1.0": we do not use the @version in Drupal, as version information is tracked by Git.
• "'access arguments' => array('administer site configuration'),": this permission is too generic, create your own.
• speed_form_blocks_forms(): what is going on this hook, you are not mapping any form callbacks here? The content does not match the purpose of hook_forms(). http://api.drupal.org/api/drupal/modules!system!system.api.php/function/...
• speed_form_blocks_callback(): "$form_state['input']": why do you need to use the raw data from $form_state['input'] and can't use validated $form_state['values']?
• speed_form_blocks_mainhelp(): why do you need that extra function and why don't you just put that into hook_help()?
• speed_form_blocks_mainhelp(): use url() to generate URLs.
• speed_form_blocks_form_creation(): this function is not called anywhere?
• speed_form_blocks_creation_finalise(): do not use check_plain() when writing to the database. "When handling data, the golden rule is to store exactly what the user typed. When a user edits a post they created earlier, the form should contain the same things as it did when they first submitted it. This means that conversions are performed when content is output, not when saved to the database." http://drupal.org/node/28984
• If you want to prefill a form element you should use #default_value, not #value. Please check that in all of your code.

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Read about the review bonus workflow: https://drupal.org/node/1410826

manual review of 3 projects -> tag your application -> you'll get a review sooner -> tag will be removed -> manual review of 3 projects -> . ..... .

BTW
ventral == drupal code sniffer

I just updated ventral.org/pareview today, are you sure your local version is up-to-date?

you tagged the issue wrong, must be "PAReview: review bonus"

corrected

Thanks for your reviews. When finishing your review comment also set the issue status either to "needs work" (you found some problems with the project) or "reviewed & tested by the community" (you found no flaws).

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

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

  
  FILE: ...dules/pareview_temp/test_candidate/speed_form_blocks.block_creation.inc
  --------------------------------------------------------------------------------
  FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
  --------------------------------------------------------------------------------
   300 | WARNING | A comma should follow the last multiline array item. Found: )
  --------------------------------------------------------------------------------
  

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:

1. speed_form_blocks_schema(): This is a hook implementation and should be documented as such: http://drupal.org/node/1354#hookimpl
2. speed_form_blocks_schema(): the closing ")" of a multiline array should be on its own line, see http://drupal.org/node/318#array
3. "'access arguments' => array('administer site configuration'),": that permissions is not a godd fit. What about "administer blocks"? Or you could of course create your own permission for your blocks.
4. speed_form_blocks_forms(): function signature does not match that from hook_forms()? Are you sure you want to implement that hook? http://api.drupal.org/api/drupal/modules!system!system.api.php/function/...
5. You are using empty lines too excessively. They should be used to group code blocks together, and not after every single statement. Currently your code is very hard to read.
6. speed_form_blocks_help(): do not concatenate translatable strings, use placeholders instead. Do not create link markup yourself, use l() instead.
7. speed_form_blocks_permission(): ah, you define your own permission, so you should use it in hook_menu().
8. speed_form_blocks_callback(): use drupal_write_record() instead of db_insert() and your data will be validated against the schema.
9. speed_form_blocks_management(): remove the access check, that is done by the menu system.
10. "'#value' => $data[$x]['type'],": do not use #value, use #default_value instead.
11. speed_form_blocks_management_handler(): do not use check_plain() on input. "When handling data, the golden rule is to store exactly what the user typed. When a user edits a post they created earlier, the form should contain the same things as it did when they first submitted it. This means that conversions are performed when content is output, not when saved to the database." From http://drupal.org/node/28984
12. speed_form_blocks_management_handler(): do not use the raw values from $form_state['input'], use $form_state['values'] instead.

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Do not RTBC your own issues.

Thank your for your reviews. When finishing your review comment also set the issue status either to "needs work" (you found some problems with the project) or "reviewed & tested by the community" (you found no flaws).

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

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

  FILE: ...ites/all/modules/pareview_temp/test_candidate/speed_form_blocks.install
  --------------------------------------------------------------------------------
  FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
  --------------------------------------------------------------------------------
   56 | ERROR | Line indented incorrectly; expected 2 spaces, found 4
  --------------------------------------------------------------------------------
  

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:

1. Your commit messages are still broken: "Signed-off-by: patlockley ...".
2. "'access arguments' => array('Administer Speed Form Blocks'),": permission does not match the one defined in hook_permission(). In general permissions should be all lower case.
3. speed_form_blocks_forms(): function signature does not match that from hook_forms(). It should have the parameters $form_id and $args.
4. "drupal_get_form("speed_form_blocks_forms", $delta)": speed_form_blocks_forms is not a form id. It is a generic hook that will map form ids. Why do you need hook_forms()? You do not do any mapping of form IDs in that function? Where do you have the requirement in your module that you would need to map several form IDs to one base form?
5. "'#title' => $description,": this is vulnerable to XSS exploits. As $description is user provided input you need to sanitize it before printing it. See http://drupal.org/node/28984
6. "$label = "Submit";": all user facing text must run through t() for translation.
7. "'#suffix' => $data->post_html . '

   ',": I think you should do at least a filter_xss() or similar here to avoid XSS. See also http://drupalscout.com/knowledge-base/drupal-text-filtering-cheat-sheet-...

8. speed_form_blocks_creation(): do not use #value on form elements, use #default_value instead. Please check that in all your form building functions.
9. speed_form_blocks_creation_finalise(): use drupal_write_record() instead of db_insert() and your data will also be validated against the DB schema.
10. speed_form_blocks_add_field_handler(): why do you call check_plain() here? You are not printing anything to the user? "When handling data, the golden rule is to store exactly what the user typed. When a user edits a post they created earlier, the form should contain the same things as it did when they first submitted it. This means that conversions are performed when content is output, not when saved to the database." from http://drupal.org/node/28984
11. "'#title' => $block->page . t("Page (click to expand)"),": Again, $block->page needs to be sanitized here.

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

This sentence is for the case you review other modules - after you've done a review of other module you have to decide whether you set their issue to needs work or rtbc.

if it's only a preference -> it's not a review -> don't change status

Please don't remove the security tag, we keep that for statistical purposes and to show examples of security problems.

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:

1. Commit messages look good now.
2. "@version 1.0": we don't use the @version tag in Drupal doxygen docs, so remove it.
3. speed_form_blocks_admin(): you don't need the user_access() check here, that is already done by the menu system.
4. speed_form_blocks_help(): $base_url is unused.
5. "'#value' => $block->pre_html,": you are still using #value in a lot of places instead of #default_value.
6. speed_form_blocks_management_handler(): again: do not use the raw POST values from $form_state['input'], use $form_state['values'] instead.

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Manual review of the 7.x-1.x branch:

1. The automated code review errors from comment #65 are not fixed. See http://drupal.org/node/1354#functions
2. speed_form_blocks_export_data(): you are still using #value instead of #default_value for the text area here.
3. speed_form_blocks_machine_name(): you are still using $form_state['input'] here. Please check all your code against the issues I already told you.

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

hi @pgogy the same thing happened to me with drupalcs and it ended out being that I needed the -dev version of drupal code sniffer. I'm attaching a dump of drupalcs for reference, it seems that there are still some minor issues.

2) Also in speed_form_blocks_export_data() it seems that it's still using #value, did you push your latest changes?

3) Seems to be correct now.

setting to needs work and keeping the pareview bonus tag for @klausi.

*ahem*
sorry... now really setting to needs work.

Hi @pgogy, I have some more issues that could help you pass a review:

whenever you're using:

'#title' => filter_xss($block->page) . " " . t("Page (click to expand)"),

you should consider changing it so it doesn't use concatenations, to avoid translation problems. A possible solution is to make it like:

'#title' => t("!page Page (click to expand)", array('!page' => filter_xss($block->page)),

If you don't need html in $block->page, you can use @page and remove the call to filter_xss.

This problem appears in lines 123 and 139 of speed_form_blocks.export.inc

I still see you're using #value instead of #default_value in lines 182 and 189 of speed_form_blocks.management.inc, and line 132 of speed_form_blocks.export.inc, and that could cause problems with the forms.

also, drupal code sniffer reports no problems!

I think after these changes this will be RTBC.

Cheers.

@pgogy cool, it looks good with pareview.sh and drupalcs. There are still some indentation and spacing issues, like too many spaces before the => and lines that go over 80 characters at:

$form['inline_feedback_main_no_responses'] = array(
      '#markup'          => '<p>' . t('No responses have been received or no blocks have been set up.') . '</p><p>' . t('If no blocks have been created yet, please visit') . ' <a href="creation">' . t('block creation') . '</a></p>',
    );

which I'd rewrite as:

$form['inline_feedback_main_no_responses'] = array(
      '#markup' => '<p>' . t('No responses have been received or no blocks have been set up.') 
        . '</p><p>' . t('If no blocks have been created yet, please visit') 
        . ' <a href="creation">' . t('block creation') . '</a></p>',
    );

Also, there are some indentation errors that you might want to correct before getting another review from @klausi.

* in speed_form_blocks.responses.inc
- lines 22, 85, 94
* in speed_form_blocks.management.inc
- lines 25, 43, 48, 59, 208.

There are also several places that need better indentation. It should always be 2 spaces, so at speed_form_blocks.responses.inc @lines 127-130 you need to add indentation for the ->condition and ->execute lines. Also, in general the indentation should be of 2 spaces.

One last thing is that the commits are still showing as "by patlockley" instead of pgogy and that should be corrected (see http://drupal.org/node/1022156).

I think the last one is a blocker so I'll set it to needs work, but as soon as that's corrected I think this is ready for RTBC!

great @pgogy, I still see some indentation issues in speed_form_blocks.management.inc around line 300:

              db_update('speed_form_blocks_created')
                ->condition('block_name', $block->block_name)
                ->fields(array(
                         str_replace($block->block_name . "_", "", $setting)
                         => filter_xss_admin($value)))
                ->execute();

and 316

            db_update('speed_form_blocks_created')
              ->condition('block_name', $block->block_name)
              ->fields(array(
                        str_replace($block->block_name . "_", "", $key)
                        => filter_xss_admin($value)))
              ->execute();

But IMHO it's good enough for a RTBC!

I think it's ok, but what about:

$field_name = str_replace($block->block_name . "_", "", $key);
db_update('speed_form_blocks_created')
  ->condition('block_name', $block->block_name)
  ->fields(array($field_name => filter_xss_admin($value)))
  ->execute();

or, if it's over 80 characters:

$field_name = str_replace($block->block_name . "_", "", $key);
db_update('speed_form_blocks_created')
  ->condition('block_name', $block->block_name)
  ->fields(array(
    $field_name => filter_xss_admin($value)
  ))
  ->execute();

anyways it's not a blocker so it's up to you.

Actually @pgogy, I was discussing it in IRC and @klausi noted that filter_xss_admin it's not supposed to be in those lines, it's supposed to be used when you're generating output, and not when saving in the database.

So please give it another look and remove the two instances of filter_xss_admin on input and please check that you're using it whenever you're outputting user generated text.

Sorry for going back to needs work, but I hope this helps getting this project published!
Also, still leaving the review bonus tag for @klausi.

@pgogy you don't need to filter for #default_value, but you must always filter #value and #markup (see http://drupal.org/node/28984).

Getting close @pgogy, the pareview.sh script returned some errors though, it seems that there are some spacing / tabs issues:

• 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.

FILE: .../sites/all/modules/custom/speed_form_blocks/speed_form_blocks.admin.inc
--------------------------------------------------------------------------------
FOUND 8 ERROR(S) AFFECTING 4 LINE(S)
--------------------------------------------------------------------------------
 14 | ERROR | String concat is not required here; use a single string instead
 15 | ERROR | Spaces must be used to indent lines; tabs are not allowed
 15 | ERROR | Line indented incorrectly; expected 2 spaces, found 1
 15 | ERROR | Array indentation error, expected 4 spaces but found 1
 18 | ERROR | Spaces must be used to indent lines; tabs are not allowed
 18 | ERROR | Line indented incorrectly; expected 2 spaces, found 1
 18 | ERROR | Array indentation error, expected 4 spaces but found 1
 19 | ERROR | Concat operator must be surrounded by spaces
--------------------------------------------------------------------------------


FILE: ...es/all/modules/custom/speed_form_blocks/speed_form_blocks.responses.inc
--------------------------------------------------------------------------------
FOUND 4 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
 94 | ERROR | Whitespace found at end of line
 95 | ERROR | Spaces must be used to indent lines; tabs are not allowed
 95 | ERROR | Line indented incorrectly; expected 4 spaces, found 3
 95 | ERROR | Array indentation error, expected 6 spaces but found 3
--------------------------------------------------------------------------------

As repay, you can consider giving reviews to other people :)

the issues found are minor and should not be required for approval, therefore please do not insist on having them fixed and do not switch the issue to needs work if there are no major issues found.

sorry @patrickd, i didn't mean to stall the issue, I actually meant to help. Thanks for clarifying that.

Hey @pgogy I think the online version is older than the latest -dev for drupalcs. As mentioned by @patrickd those are minor issues, so it's up to you I guess.. What I did is I tried to remove all errors and warnings just in case, but it shouldnt block your application.

On my end, I've pulled your changes and pareview.sh and drupalcs seem happy with your code.

This is set to needs review and you have a pareview bonus so you just need to wait to get a review from @klausi.

manual review:

1. speed_form_blocks_menu(): $path is unused.
2. speed_form_blocks_form_maker(): function parameters are wrong: the first should be $form, the second $form_state and the third $delta. Then you don't need to access $delta['build_info']['args'][0] and you can use $delta.
3. "$button = filter_xss($data->button);": the title of a button should always be plaintext, no? So you should use check_plain() instead.
4. There are still a lot of useless newlines in your code that do not separate code blocks, which is bad for readability.
5. speed_form_blocks_machine_name(): you should use the machine_name form element instead, then you don't need this function. See http://api.drupal.org/api/drupal/developer!topics!forms_api_reference.ht...
6. speed_form_blocks_creation_name_exists(): the "%" placeholder will sanitize the variable for you, so the filter_xss() is not needed.
7. speed_form_blocks_creation(): doc block: The first line of the block should begin with a verb in the form "Does such and such", i.e. "Handles the Block Creation Admin Page." instead of "This function handles the Block Creation Admin Page.". Also elsewhere. See http://drupal.org/node/1354#functions . @param and @return for the standard parameters and return value should be omitted from all form-generating functions. See http://drupal.org/node/1354#forms
8. speed_form_blocks_management(): another filter_xss() where not necessary ("@" placeholder). Also in speed_form_blocks_responses() and possibly elsewhere.

Although you should definitively fix those issues they are no blockers, so I think this is RTBC. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Thanks for your contribution and welcome to the community of project contributors on drupal.org!! :)

I've granted you the git vetted user role which will let you promote this to a full project and also create new projects as either sandbox or "full" projects depending on which you feel is best.

Thanks, also, for your patience with the review process and your help in the application queue. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

As you continue to work on your module, keep in mind: Commit messages - providing history and credit and Release naming conventions.

Thanks to the dedicated reviewer(s) as well.

As per #94 you reached the end of this application, congratulations, please leave it fixed ;-)

Review bonus is not needed anylonger - this issue is finished - your application is approved - see comment #94

As successful completion of the project application process results in the applicant being granted the ´Create Full Projects´ permission, there is no need to take multiple applications through the process. Once the first application has been successfully approved, then the applicant can promote other projects without review. Because of this, posting multiple applications is not necessary, and results in additional workload for reviewers. See documentation about full projects (and sub-pages).