markoshust [markoshust]

Hello, and thank you for applying for a CVS account. I am adding the review tags, and some volunteers will review the code, pointing out what it needs to be changed.

1. Menu descriptions and titles, schema descriptions are not passed to t().

2. You are strongly encouraged to always use curly braces even in situations where they are technically optional. Having them increases readability and decreases the likelihood of logic errors being introduced when new lines are added. http://drupal.org/coding-standards

function simplify_form_alter(&$form, &$form_state, $form_id) {
  switch ($form_id) {
    case "page_node_form":
      if (!variable_get('simplify_node_menu'))
        unset($form['menu']);
      if (!variable_get('simplify_node_revision_information'))
        unset($form['revision_information']);
      if (!variable_get('simplify_node_path'))
        unset($form['path']);
      if (!variable_get('simplify_node_comment_settings'))
        unset($form['comment_settings']);
      if (!variable_get('simplify_node_author'))
        unset($form['author']);
      if (!variable_get('simplify_node_options'))
        unset($form['options']);
  }
}

Coder found 1 projects, 2 files, 2 normal warnings, 2 minor warnings.

simplify.module

    *
      severity: normalLine 41: Menu item titles and descriptions should NOT be enclosed within t(). (Drupal Docs)

          'title' => t('Simplify'),

    *
      severity: normalLine 42: Menu item titles and descriptions should NOT be enclosed within t(). (Drupal Docs)

          'description' => t('Adjust Simplify configuration options.'),

Hide sites/all/modules/simplify/simplify.install
simplify.install

    *
      severity: minorLine 11: There should be no trailing spaces

       * 

    *
      severity: minorLine 25: There should be no trailing spaces

       * 

Please use Coder and correct the warnings.

Tested this module on Drupal 7, and it seems to work.

---------

But I don't really see the use of this. It removes this options ONLY for page nodes and for every user, even the admin user (uid 1). If a users that creates any types of nodes shouldn't see this options, it can be done with Permissions.

      // Unset specified form items if and only if variable is 0 (false).
      if (0 === variable_get('simplify_node_menu')) {
        unset($form['menu']);
      }
      if (0 === variable_get('simplify_node_revision_information')) {
        unset($form['revision_information']);
      }
      if (0 === variable_get('simplify_node_path')) {
        unset($form['path']);
      }
      if (0 === variable_get('simplify_node_comment_settings')) {
        unset($form['comment_settings']);
      }
      if (0 === variable_get('simplify_node_author')) {
        unset($form['author']);
      }
      if (0 === variable_get('simplify_node_options')) {
        unset($form['options']);
      }

By reading your description about the module above and when I saw this code I got very confused. I get this feeling that if simplify_node_options is true (1) then the $form['options'] will be unset, part by the description you gave and by the variable name. But when I read "Show publishing options" as the title for the checkbox it came clear to me. This might only be me, but this was not easy to understand. More comments or new variable names, or change from "Show" to "Hide publishing options" would make this code better. But as I said, this might only be me that think like this, so this is only a suggestion from my side.

if (0 === variable_get('simplify_node_options')) {
        unset($form['options']);
      }

I would expect too that the code should be, basing on the name of the variable:

if (variable_get('simplify_node_options')) {
        unset($form['options']);
      }

In all that code lines is not necessary to strictly compare the variable value with 0 (or 1).

1.
package = Other
This is default, not needed.

2.
version = 7.x-1.0
This is added by the CVS, so remove it.

3.

  $form['simplify'] = array(
    '#type' => 'fieldset',
    '#title' => t('Nodes'),
  );

Think the title 'Nodes' are misleading, because your module only effects on the content type 'page'.

-----

There is no documentation about that this module only effects content type 'page', only in your first post here. I would like to see a note in a readme file or in the settingpage for this module.

1. Use drupal_substr in line 84 in .module

2. Why declare global $user; two times in function simplify_form_alter, why not put it in the begining in the function instead?

3. Why do you first use a switch for only one case, and after a if in simplify_form_alter? Why not combind those two to a switch or if/else if/else?

4. What if the content type 'page' dose not exists, then I think there should be no settings for that type. Or what do you think?

Sry, the link was to D6, and this module is D7, but think you got it.

Forgot to change status too.

1. Good.

2. It make sence, yes ;). I might have writen it to fast, because there is no right or wrong way to do that, what I know. But you will save lines of code with only declare it ones or you will save a microsecond or so if you put it in the if or switch. So do it the way you want.

3. Good.

4. You can use node_get_types() to get a list of content types and check.

1. Use this function instead, node_type_get_types(). Know that thoose are (almost) equal, but this it the one to use.

I will be actively updating this module on a regular basis with new features and capabilities once it is released.

Have you made any updates since your last attachment?

Okey. The modules code looks ok and it seems to do what it should do.

Thank you for your contribution! I am going to update your account.

These are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find more contributors chatting on the IRC #drupal-contribute channel. So, come hang out and stay involved.
Thank you, 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.

I thank all the dedicated reviewers as well.