[D7] Lesser Forms

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxNovitsh2274341git

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

README.txt or README.md is missing, see the guidelines for in-project documentation. (It is present though?)

No README.txt in the repo (only README.TXT).

Also better get this fixed: 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.

These may look like just nits to you ... However, the issue queue is long and at least some of us use scripts to automate some of the tasks involved (such as cloning the project environment for testing). Applications that doesn't have a major version branch, a working git clone command, break coding standards and make the code hard to read, or comes with a non-standard README.txt ... are just sand in the machinery. These are not blocking issues, but having them wrong lessens you chances of getting an initial review.

Initial observations

Individual user account
Yes: Follows the guidelines for individual user accounts.
Git Clone Command
No. The correct is git clone --branch 7.x-1.x-alpha http://git.drupal.org/sandbox/Novitsh/2274341.git lesser_forms/
Licensing
Yes: Follows the licensing requirements
3rd party code/content
Yes: Follows the guidelines for 3rd party code.
Project page
Yes
README.txt
Yes

I suggest using db_select rather than in _lesser_forms_read_all_user_roles db_query ()

Remove the comment line

56  // $form['menu']['#access'] = FALSE; 

I have created a 7.x-1.x-alpha branch.

Unfortunately, 7.x-1.x-alpha, is not a valid branch name.

The correct branch name for your project is: 7.x-1.x. Please change the name of the default branch to this. Then add the correct git clone command to your issue summary. The correct git clone command is:

git clone --branch 7.x-1.x http://git.drupal.org/sandbox/Novitsh/2274341.git lesser_forms

Also read the following: Moving from a master branch to a version branch, Release naming conventions and Creating a branch in git - and make sure you understand it.

I took a look at your code, and I have some suggestions for you:

Modify your variables so that they are part of one pattern called lesser_forms_config, ie

$settings = variable_get('lesser_forms_config', array());

In your admin form, if you make liberal use of '#tree' => TRUE in your containers, you can address your values like so:

In _lesser_forms_configuration_submit()

$settings = variable_get('lesser_forms_config', array());
$settings = array_merge($settings, $form_state['values']['lesser_forms_config']);
variable_set('lesser_forms_config', $settings);

Then you can address them in lesser_forms_form_alter() by index.

Instead of

if (variable_get('lesser_forms_options_promote', FALSE)) {
      hide($form['options']['promote']);
}

You could do:

if(isset($settings['options']['promote']) && $settings['options']['promote'])
{
   hide($form['options']['promote']);
}

YMMV though.

I went through and did an automated and manual review.

Automated Review
The parareview.sh showed no errors in coding standards.

Manual Review

README.txt
Please take a moment to make your README.txt follow the guidelines for in-project documentation. You can find a great template for readme files here

";" At the end of Function Closing Brackets
This is super nit picky, but at the end of your functions closing brackets you have a ";" which isn't needed. You can remove these as they aren't needed.

db_select vs api call
In your _lesser_forms_read_all_user_roles you have a db_select() which appears as though it getting the role names. There is a api function you may want to consider using instead called "user_roles()" which looks like it would return exactly the informatino you need. It's generally better to stick to using the built in API calls if at all possible.

Otherwise, I like the module and as someone who is a big fan of usability improvements this gets a thumbs up from me. Great job @Novitsh

Some of my initial comments:

This form alter will be executed by all forms. It should be specific to forms.

Either make use of $form_id variable or hook_form_FORM_ID_alter

https://api.drupal.org/api/drupal/modules%21system%21system.api.php/func...

/**
 * Implements hook_form_alter().
 */
function lesser_forms_form_alter(&$form, &$form_state, $form_id) {

Getting a lot of Notices on configuration page.

Hi Novitsh,

Thanks for you contribution.
I still can see the notices on admin page reported above in comment #20.
Please see below:

You can populate default values at line # "$settings = variable_get('lesser_forms_config', array());" to avoid notices (as in one go they looks like errors).

One more thing, I was wondering If I do not select any user roles then what will rest of settings will do?? I think for selecting any setting, selection of user roles should be mandatory.

Thanks!!

Doing a quick code review:

lesser_forms.module:

1. There are multiple indentation errors: some lines are not indented, tabs are used instead of spaces. See Indenting and whitespace.
2. lesser_forms_form_alter() has a serious bug: this code is running on ALL forms and will hide certain root level elements in the form, regardless of their context (e.g. $form['preview'] and $form['author']). This might have very unexpected consequences if this code runs on random forms that might use the same keys. You should use hook_form_FORM_ID_alter() or hook_form_BASE_FORM_ID_alter() instead.
3. Don't use abbreviations in variable names. For example instead of $usr_roles, use $user_roles.

4. $usr_roles = (isset($settings['appliesto'])) ? $settings['appliesto'] : array();
   

   You don't need to wrap the isset() in parentheses.

5.   foreach ($usr_roles as $role => $id) {
       if ($id) {
         if (in_array($role, $user->roles)) {
           $usr_match = TRUE;
           break;
         }
       }
     }
   if ($usr_match) {
     // ...
   }
   

   This is a rather long winded way to check if the roles are present for the user. PHP has built-in support for this, no need to loop. This whole section can be replaced with a one liner:

   if (array_intersect(array_keys(array_filter($usr_roles)), $user->roles)) {
     // ...
   }
   

lesser_forms.admin.inc:

1. The main form builder is called _lesser_forms_configuration(). Form builders do not typically start with an underscore, and they usually end in _form() to make their purpose clear.
2. Please use the standard documentation for form builders. See Documenting form generating functions.
3. For standard configuration forms that save their result in a variable it is better to use system_settings_form(). This takes care of saving and loading the variables, so you don't need the submit handler.

I'm sorry to say but I'm not convinced that this module is useful for Drupal users in general. This hides some specific fields in the node edit form, which is a very common use case that typically is implemented in a custom module according to a client's specific needs.

Novitsh: that's the bug I mean, the keys that are used in form arrays are arbitrary. I'll give you an example where this can go wrong.

Imagine I have a company called "Bookgeeks" that organises book signing events. My site has a custom form that allows me to plan my events:

function bookgeeks_author_signing_session_form($form, &$form_state) {
  // An autocomplete field to select from available authors.
  $form['author'] = array(
    // ...  
  );

  // The date the event takes place.
  $form['date'] = array(
    // ...
  );

  return $form;
}

I now enable your module because I want to simplify my node forms and no longer show the author field. Suddenly this form which is critical to my business will be broken.

Nice looking project. I checked it over and see no major issues. One stylistic change is your function documentation should include information about the parameters passed to them and the return types (per https://www.drupal.org/node/1354).

I can see adding this module to my own site.

Hi,
first of all thanks for this interesting looking module! Here's my review:

PAReview / code style

There are several code style errors, when doing a PAReview (http://pareview.sh/pareview/httpgitdrupalorgsandboxnovitsh2274341git). Although this stuff is non-functional, sticking to a unified code style is very important imho. And the best thing: these errors are all very easy to fix.

Especially, please rename README.TXT to README.txt. Other automated tools may also won't find the file, if the suffix is uppercased.

When defining variables or array keys having multiple words, it's better for readability to use underscores in the keys/names, instead of mergin them (e.g. "applies_to" is better than "appliesto").

Please remove the underscore from the start of your form function name (_lesser_forms_configuration). This is unusual and could lead to problems for developers trying to alter the form (as they would have a double underscore in their form_alter implementation). Also it is common practice to end the function name with "_form" to indicate that this is a form builder function. (Please cascade the change to validate and submit functions as well)

Currently, your form validation function does nothing than just looping through the submitted values, but never really running any validation. So you could drop the function completely at the moment. Is there any validation planned?

I'm not 100% sure, if there is an official recommendation, but I'd prefer setting '#access' to FALSE on a form field than calling hide(), although the effect is the same. And one time you do set access to FALSE instead of hiding it. So at least, you're not consistently using the same approach.

info file

Please add the following line to your info file, in order to show the configuration link on the module administration page:
configure = admin/config/content/lesser_forms

You may state in your readme file that the configuration does only have an impact on user roles with "administer content" permission enabled.

PHP warnings/notices

When I enter the configuration page of your module the first time (immediately after installation), I'm getting this notice six times:
"Notice: Array to string conversion in form_process_checkbox() (Zeile 3217 von /www/htdocs/w00c1ab7/website-sandbox/test-custom-build/drupal/includes/form.inc)."

This is because all your default values for your checkbox are defined as arrays instead of empty strings.

Functionality

Unfortunately, it was impossible for me to disable the version information - the other fields/tabs worked as expected.

Conclusion

Still some work to do, but it won't be too much work to fix the remaining issues.

In general, I like the idea behind your project very much. The gap in D7 between the rights you get with "administer content" permission and not having them, is quite big. Often you just want a single permission, but have to assign all or nothing. Your module fixes that :-) It's potentially one of these little modules, that you put in your default installation profile and add to every site you build :-)

A feature request (for future versions): define per role which fields to hide. For more complex sites with different roles having editing permission, this would be nice to have. Additionally in this case, it would be worth considering, dropping the configuration on a variable base in favour of defining different permissions.

PAReview.sh reports nothing but a shortish git commit message and a missing test suite. Everything else checks out.

I have done a full reading on the code and found no issues. Nicely formatted, clearly commented, no glaring mistakes. All the issues pointed out by previous reviews have been handled.

Having hard coded something similar to the functionality of this module on numerous occassions, it is my opinion that this approach solves the problem in a simple, no-nonsense manner that achieves the objective effectively.

Good to go.

Review of the 7.x-1.x branch (commit 889f9a7):

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

  FILE: /home/klausi/pareview_temp/admin/lesser_forms.admin.inc
  -------------------------------------------------------------------------
  FOUND 1 ERROR AFFECTING 1 LINE
  -------------------------------------------------------------------------
   6 | ERROR | [x] Doc comment short description must be on the first line
  -------------------------------------------------------------------------
  PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
  -------------------------------------------------------------------------
  
  FILE: /home/klausi/pareview_temp/lesser_forms.module
  -------------------------------------------------------------------------
  FOUND 1 ERROR AFFECTING 1 LINE
  -------------------------------------------------------------------------
   6 | ERROR | [x] Doc comment short description must be on the first line
  -------------------------------------------------------------------------
  PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
  -------------------------------------------------------------------------
  
  FILE: /home/klausi/pareview_temp/lesser_forms.install
  -------------------------------------------------------------------------
  FOUND 1 ERROR AFFECTING 1 LINE
  -------------------------------------------------------------------------
   6 | ERROR | [x] Doc comment short description must be on the first line
  -------------------------------------------------------------------------
  PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
  -------------------------------------------------------------------------
  

• No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

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. You have to get a review bonus to get a review from me.

manual review:

1. I think "Lesser forms" is not a good module name, since it does not describe what the module does. I would use "Hide node options" or "Slim node form" or similar.
2. lesser_forms_form_alter(): so this will trigger on all forms on the site? Shouldn't you only do this on node edit forms and check the form ID?
3. lesser_forms_configuration_form(): do not document $form and $form_state here since that is the same for all forms, see https://www.drupal.org/coding-standards/docs#forms . Same for lesser_forms_configuration_form_submit().
4. lesser_forms_configuration_form_submit(): why the array_merge() here? Just save the values as is form the form_state? Or has that a special purpose? Please add a comment.
5. "if (isset($settings['promote']) && $settings['promote']) {": you can just use !empty() instead of the two conditions.

But that are not critical application blockers, so ...

Thanks for your contribution, Novitsh!

I updated your account so you can promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here 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 lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and stay involved!

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.

Thanks to the dedicated reviewer(s) as well.

You created the sandbox as "drupal.org project" but you should have created it as "module" project. Please create a new sandbox, push the git commits there and copy your project page. Then you should be able to promote the sandbox (and you should delete the old one after that).