Semantic Blocks

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:

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: ...al-7-pareview/sites/all/modules/pareview_temp/test_candidate/README.txt
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AND 7 WARNING(S) AFFECTING 7 LINE(S)
--------------------------------------------------------------------------------
2 | WARNING | Line exceeds 80 characters; contains 104 characters
3 | WARNING | Line exceeds 80 characters; contains 126 characters
8 | WARNING | Line exceeds 80 characters; contains 104 characters
9 | WARNING | Line exceeds 80 characters; contains 127 characters
10 | WARNING | Line exceeds 80 characters; contains 135 characters
13 | WARNING | Line exceeds 80 characters; contains 104 characters
14 | WARNING | Line exceeds 80 characters; contains 97 characters
14 | ERROR | Files must end in a single new line character
--------------------------------------------------------------------------------

FILE: ...7-pareview/sites/all/modules/pareview_temp/test_candidate/block.tpl.php
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AND 3 WARNING(S) AFFECTING 4 LINE(S)
--------------------------------------------------------------------------------
15 | WARNING | Line exceeds 80 characters; contains 82 characters
17 | WARNING | Line exceeds 80 characters; contains 84 characters
18 | WARNING | Line exceeds 80 characters; contains 82 characters
42 | ERROR | File doc comments must be followed by a blank line.
--------------------------------------------------------------------------------

FILE: ...iew/sites/all/modules/pareview_temp/test_candidate/semantic_blocks.info
--------------------------------------------------------------------------------
FOUND 3 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
5 | ERROR | It's only necessary to declare files[] if they declare a class or
| | interface.
6 | ERROR | Files must end in a single new line character
6 | ERROR | It's only necessary to declare files[] if they declare a class or
| | interface.
--------------------------------------------------------------------------------

FILE: .../sites/all/modules/pareview_temp/test_candidate/semantic_blocks.install
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
9 | ERROR | Missing function doc comment
39 | ERROR | Files must end in a single new line character
--------------------------------------------------------------------------------

FILE: ...w/sites/all/modules/pareview_temp/test_candidate/semantic_blocks.module
--------------------------------------------------------------------------------
FOUND 62 ERROR(S) AND 1 WARNING(S) AFFECTING 54 LINE(S)
--------------------------------------------------------------------------------
2 | ERROR | You must use "/**" style comments for a file comment
4 | WARNING | Line exceeds 80 characters; contains 82 characters
11 | ERROR | Whitespace found at end of line
12 | ERROR | Missing parameter type at position 1
13 | ERROR | Parameter comment indentation must be 2 additional spaces at
| | position 1
14 | ERROR | Function doc comment must end on the line before the function
| | definition
18 | ERROR | No space before comment text; expected "// load the element as
| | a string" but found "//load the element as a string"
18 | ERROR | Inline comments must start with a capital letter
18 | ERROR | Inline comments must end in full-stops, exclamation marks, or
| | question marks
26 | ERROR | Whitespace found at end of line
27 | ERROR | Missing parameter type at position 1
28 | ERROR | Parameter comment indentation must be 2 additional spaces at
| | position 1
38 | ERROR | Whitespace found at end of line
42 | ERROR | No space before comment text; expected "// check user access"
| | but found "//check user access"
42 | ERROR | Inline comments must start with a capital letter
42 | ERROR | Inline comments must end in full-stops, exclamation marks, or
| | question marks
44 | ERROR | Whitespace found at end of line
45 | ERROR | Line indented incorrectly; expected 4 spaces, found 2
46 | ERROR | Line indented incorrectly; expected at least 6 spaces, found 4
47 | ERROR | Line indented incorrectly; expected at least 6 spaces, found 4
48 | ERROR | Line indented incorrectly; expected at least 6 spaces, found 4
49 | ERROR | Line indented incorrectly; expected at least 6 spaces, found 4
51 | ERROR | Line indented incorrectly; expected at least 6 spaces, found 4
57 | ERROR | Line indented incorrectly; expected at least 6 spaces, found 4
58 | ERROR | Line indented incorrectly; expected at least 6 spaces, found 4
63 | ERROR | No space before comment text; expected "// load the elements"
| | but found "//load the elements"
63 | ERROR | Inline comments must start with a capital letter
63 | ERROR | Inline comments must end in full-stops, exclamation marks, or
| | question marks
65 | ERROR | Line indented incorrectly; expected at least 6 spaces, found 4
66 | ERROR | Line indented incorrectly; expected at least 6 spaces, found 4
68 | ERROR | Closing brace indented incorrectly; expected 2 spaces, found 0
69 | ERROR | Whitespace found at end of line
74 | ERROR | Whitespace found at end of line
78 | ERROR | No space before comment text; expected "// check user access"
| | but found "//check user access"
78 | ERROR | Inline comments must start with a capital letter
78 | ERROR | Inline comments must end in full-stops, exclamation marks, or
| | question marks
80 | ERROR | Line indented incorrectly; expected 4 spaces, found 2
81 | ERROR | Line indented incorrectly; expected 6 spaces, found 4
82 | ERROR | Line indented incorrectly; expected at least 8 spaces, found 6
83 | ERROR | Line indented incorrectly; expected at least 8 spaces, found 6
84 | ERROR | Line indented incorrectly; expected at least 8 spaces, found 6
85 | ERROR | Line indented incorrectly; expected at least 8 spaces, found 6
86 | ERROR | Line indented incorrectly; expected 8 spaces, found 6
87 | ERROR | Line indented incorrectly; expected at least 10 spaces, found
| | 8
91 | ERROR | Closing brace indented incorrectly; expected 2 spaces, found 0
94 | ERROR | Whitespace found at end of line
96 | ERROR | Whitespace found at end of line
97 | ERROR | Data type of return value is missing
98 | ERROR | Return comment indentation must be 2 additional spaces
101 | ERROR | Line indented incorrectly; expected at least 2 spaces, found 0
101 | ERROR | An operator statement must be followed by a single space
103 | ERROR | Whitespace found at end of line
104 | ERROR | Whitespace found at end of line
105 | ERROR | Whitespace found at end of line
109 | ERROR | Whitespace found at end of line
110 | ERROR | Whitespace found at end of line
111 | ERROR | Whitespace found at end of line
112 | ERROR | Whitespace found at end of line
113 | ERROR | Array closing indentation error, expected 4 spaces but found 6
115 | ERROR | Line indented incorrectly; expected at least 2 spaces, found 0
121 | ERROR | Whitespace found at end of line
127 | ERROR | Whitespace found at end of line
131 | ERROR | Files must end in a single new line character
--------------------------------------------------------------------------------

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

FYI
with a branched called "7.x-1.x-dev" you'll not be able to create a release later, it has to be 7.x-1.x for that (see naming conventions http://drupal.org/node/1015226) surely you can create this branch later, but it would be better if you'd follow the conventions to avoid confusion

Hi,
I looked over your module and saw that on the block configuration page, your config is the first one, above the title. I think you should move it down a bit.
Looking into the code, line #65, you have a function named

function semantic_blocks($block) { ... }

I'm not sure if it's a good idea to have a function with the same name as the module itself. You should probably change it to something like

function semantic_blocks_get_elements($block) {}

I don't know if it's a bug or I have some other stuff to configure, but for the development block I chose the aside tag and I didn't saw it on page. See screenshots attached.

• put your .gitignore into your .gitignore
• it's a strange phenomenon: many applicants have few information on the project page, but much information in their readme. The clue: nobody looks into readme's, but everyone complains about bad project pages
  could you get it managed to write more information on your project page? ;-)
• You should take more care about intedation, eg in:
  

    $ret = db_query('SELECT element FROM {semantic_blocks} WHERE module = :module
    AND delta = :delta', array(
    ':module' => $block->module,
    ':delta' => $block->delta)
    )->fetchField();

• On longer variable definitions, strings, etc you don't have to take care about the 80 characters rule if your code is going to look ugly like this:
  

  $description = t('Select the HTML element to use as the wrapper for this
        block.');

• '#description' => $description,
  doing this is also not necessary if the description is only used once!
• two little style issues
  

  FILE: ...w/sites/all/modules/pareview_temp/test_candidate/semantic_blocks.module
  --------------------------------------------------------------------------------
  FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
  --------------------------------------------------------------------------------
  68 | ERROR | Array indentation error, expected 4 spaces but found 2
  69 | ERROR | Array indentation error, expected 4 spaces but found 2
  --------------------------------------------------------------------------------

I actually wanted to do this more intensive, but I'm getting tired :/

As I only found some minor stuff, I won't switch to needs work and have another look tomorrow.

Generally I'm sorry for the delay!
as there are currently many applications in queue we need more reviewers,
so think about getting a review bonus and we will come back to your application sooner.

regards

yes, putting the gitignore into the gitignore make git ignore the gitignore :D

soo let's get picky
(many of these points are suggestions, don't follow them blind, feel free to ask)

1. The core blocks module is optional, so you should make a dependency on it.

2.  /**
     * Implements hook_schema().
     */
   function semantic_blocks_schema() {
   

   you indented here with 1 space too much

3. for module column is varchar(64) enough (see core blocks)
4. same with delta -> varchar(32)
5. please think about using DTNG queries (eg in _get_element()) instead of these old ugly sql strings ;) (you can have a look at the dbtng examples module)

6. function semantic_blocks_form_alter(&$form, &$form_state, $form_id) {
     // Check user access.
     if (user_access('select block element')) {
   
       if ($form_id == 'block_admin_configure' ||
       $form_id == 'block_add_block_form') {

   Unfortunately it's not easy to use the more performat hook_form_FORM_ID_alter here because you have to react on 2 form-id's.. but anyway you should only check whether the user has access if you're sure you got the right id to work with.

7. But, moreover.. is it really necessary to create a permission for selecting a html5 element for a block? I'm pretty sure the existing permission admininister blocks would do the same here ;)

8. t('address'),
   t('article'),
   t('aside'),

   why are you translating html-tags?
   if you really want to do that you should at least do it this way:

   'address' => t('address'),
   'article' => t('article'),
   'aside' => t('aside'),

   So the tags are only translated for displaying them in the select box but not when including them into the template

9.  $block->module = $form['module']['#value'];
   $block->delta = $form['delta']['#value'];

10. semantic_blocks_get_element($block)
    

    maybe it would be better to use semantic_blocks_get_element($module, $delta) instead of working around of it?

11. Also the names of semantic_blocks_form_alter and semantic_blocks_form_submit are very general.. maybe you could choose a more obvious name for them ? (As they have a very specific function)

12. function semantic_blocks_form_submit($form, &$form_state) {
      // Check user access.
      if (user_access('select block element')) {

    same access stuff here, I don't think it's necessary to check again after submit. (this would all become easier if you use administer blocks instead)

13. db_delete('semantic_blocks')->condition('module',
    $module)->condition('delta', $delta)->execute();

    I see indenting issues in several places - where you break lines - you should indent

14. function semantic_blocks_menu() {
      $items['admin/config/development/semantic_blocks'] = array(
        'title' => t('Semantic Blocks Settings'),
        'description' => t('Enable overriding the block template.'),

    do never ever use t() in hook_menu - seriously! ;)

15. variable_get('semantic_blocks_override_template', 0),if you're using variables - delete them in your install file with hook_uninstall and variable_del

16. '#description' => t('Replaces block.tpl.php with modified version to output the
        selected tag. Please refer to the help section if your theme or other module uses
        a custom template. ONLY use this if there is no other module / theme modifying
        the block template.'),

    Coding standarts say you don't have to break code in variable definitions when it's getting ugly!

17.  * Configuration form
     *
     * @return form
     *   Drupal form

    all comments should and with . / ! / ?
    you don't have to document the returning value - if you're just saying it's a "Drupal form" ;)

18. if (variable_get('semantic_blocks_override_template') == 1) {When using booleans you should always use TRUE or FALSE

that's it! phew!

Oh, you're right with the function names .. (seems like I was very tired yesterday^^) - forget this one

Me personally would rather see less permissions, but okay if you're sure it's needed - why not

A few points I recognized while having a quick look

• the gitignore is still there.. actually I was sure it would be ignored.. could you try deleting - committing and re-adding it? maybe it's not ignored because it's already "git add"ed

• function semantic_blocks_form_submit($form, &$form_state) {
    $formid = $form_state['values']['form_id'];
    if ($formid == 'block_admin_configure' || $formid == 'block_add_block_form') {
  

  As you only add your submit handler when you already made sure it's a block add/edit form you don't have to check whether it's the right form again in your submit handler - in other cases it would never be called (I'm not sure if drupal detects and uses this submit handler automatically because it has the same name as the hook_form_alter with _submit - you should test this - if it's the case you should rename the submit handler)

• variable_get('semantic_blocks_override_template', 0),Also here's a boolean - use FALSE !

I don't tested the functionality again, I think it should work..
Please fix those issues mentioned above, but anyway I think this is ready..

Please concider to get a review bonus and we will come back to your application sooner!

I'm changing the priority back to normal - you can raise it again if you (hopefully not) waited for another 2 weeks.

regards

I think I do need to check the block ID here again as hook_form_submit will be called on all form_submits.

there is no hook_form_submit, that's the reason why you should rename it.
In the hook_form_alter you add your custom submit handler only if you already verified you're handling the right form
-> the submit handler should only be called when it's added as submit handler
-> if it's called anyway you should rename it to eg. _block_form_submit() so it'll be definitely only called when you added it

that's what I meant in #16 (sorry it's kind of hard to explain that ^^)

regards

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

• Remove all .project files from your repository.
  

  .project
  

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:

• semantic_blocks.module: there is a confusing additional doc block after the @file doc block?
• semantic_blocks_preprocess_block(): indentation errors, always use 2 spaces per level.
• semantic_blocks_save_element(): you should use drupal_write_record() instead of db_insert().
• "'access arguments' => array('access administration pages'),": this permission is too generic, create your own.
• "Configuration form": All documentation and comments should form proper sentences and use proper grammar and punctuation.
• Reviews of other project applications are missing in your issue summary. It is strongly recommended to do them, see https://drupal.org/node/1011698

Hi Eric,

I have just completed reviewing your module and it would appear that most, if not all, of the points above have been addressed.

There are no licensing issues with the code as no third party files are included.

An automated review did not find any issues, and on a closer manual inspection I found the following:

Your hook_menu() function specifies the admin path as:

$items['admin/config/development/semantic_blocks'] = array(

and you refer to this path in your .info file, as per:

configure = admin/config/development/semantic-blocks

This does not actually work as there is a mismatch between the two - you have used an underscore in hook_menu() and a hyphen in the configure link in .info. Personally I would update the hook_menu() path to use a hyphen, as that seems to be the norm in Drupal.

Assuming that is fixed, which I am sure it will be ;-) I see no reason to go RTBC with this module.

drupal_write_record() validates against the schema of the table and has some sanity checks built in, so you can consider it more safe than a raw db_insert(). Sorry for not mentioning that.

Thanks for your contribution, ericgsmith! 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. 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.