[D7] User Revision Edit

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.

Hi DonSchoe,

That's certainly an interesting module, if I don't exactly agree with the idea of changing previous revisions of anything.

Drupalcs automated testing has picked up some errors:

FILE: ...t/sites/all/modules/review/user_revision_edit/user_revision_edit.module
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
 278 | ERROR | Function comment short description must end with a full stop
 334 | ERROR | The second argument to watchdog() should not be enclosed with
     |       | t()
--------------------------------------------------------------------------------


FILE: ...ser_revision_edit/views/views_handler_field_user_revision_edit_link.inc
--------------------------------------------------------------------------------
FOUND 3 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
 11 | ERROR | Class name must begin with a capital letter
 11 | ERROR | Class name must use UpperCamel naming without underscores
 32 | ERROR | Public method name
    |       | "views_handler_field_user_revision_edit_link::render_link" is not
    |       | in lowerCamel format, it must not contain underscores
--------------------------------------------------------------------------------

You should split the administration part of this module in a spearate user_revision_edit.admin.inc file.

You shouldn't adhere so strictly to the 80 characters per line rule. Some parts would be more readable if you don't break the line.

Module info file lines 7-11
You don't have any tests, so you shouldn't have any test dependencies either.

In module file, at lines 4, 24, 43, 85, 238, 278, 296, 303, 413, 428, 434 you have links to pages on drupal.org. There should only be links, if something is particularly confusing, and you need to link to a page which will explain why this is, or you need to link to a page that documents certain behavior. And the syntax is:
* @see http://drupal.org/node/2270833

On line 74, the check for 'node_admin_theme' variable is not correct. What do user revisions have to do with a configuration setting for nodes?

Lines 99-126 the checking of permissions can be a lot more compact, and easily readable:

$revert_permission = user_access('revert user revisions')
  || $user->uid == $viewed_user->uid && user_access('revert own user revisions');

On line 222, inside a form constructor, you should use form_load_include().

On line 330, why is the cache_clear_all() call needed? I don't think it is.

On line 31 and 364 _user_revision_edit_access() is only used on one place, so you shouldn't pass permissions as arguments. I haven't seen permissions being passed as array to an access callback like that. You can explicitly check for the two:

$access = user_access('edit own user revisions')
  || $user->uid == $u->uid && user_access('edit user revisions');

You should change the argument name from $u to something more descriptive.

Hi donSchoe,

Lets review the items one by one:

1. Automated test. As you said, the views part is false positive, but you misunderstood the message of one test:
   

   The second argument to watchdog() should not be enclosed with t()

   Remove t(). First argument of t() should become the $message of watchdog(), and second argument - the $variables.

2. When I said:
   

   You should split the administration part of this module in a spearate user_revision_edit.admin.inc file

   I meant to move every page callback in hook_menu and hook_menu_alter to be in separate file(s). This means:

   • user_revision_edit_form
   • user_revision_edit_overview + all validate/submit callbacks

   In hook_menu and hook_menu_alter add 'file' => 'user_revision_edit.admin.inc',
   And move hook_admin_paths() back to module, or it won't work.

3. About 'node_admin_theme'. You're correct and I was wrong.
4. 
   

   I changed all references to drupal.org issues to // @see.

   Syntax is correct, but I was thinking that these references should be removed. Because, is there any reason for a future you, or other developer looking at the code, to see these? If you need to track what change was introduced where, that's what git history and commit messages are for.

5. About the 80 characters thing, ignore what I said. This is your code style, and it follows guidelines. Beyond this, it is a matter of preference.
6. About form_load_include(). I didn't mention, but in user_revision_edit_form() you might need for &$form_state to be passed by reference for form_load_include() to really affect $form_state.
7. 
   

   I simplyfied the permission checks as suggested in access callback.

8. About needing to clear the cache - fair enough.
9. 
   

   Changed $u and $perm to $account and $permissions.

Anyway, I am satisfied with this module, and I am sure you will fix these small issues that are left. I hope you get more reviews soon and be given permissions to create full projects. Good luck :)

It's not call-time. I meant for the form constructor to get &$form_state by reference:

function user_revision_edit_form($form, &$form_state, $user_revision) {

Other than that, I am satisfied with the changes.

Hi donSchoe,

First of all: Thanks for your contribution.

Automated Review

FILE: ...iew/pareview_temp/views/views_handler_field_user_revision_edit_link.inc
--------------------------------------------------------------------------------
FOUND 3 ERRORS AFFECTING 2 LINES
--------------------------------------------------------------------------------
11 | ERROR | Class name must begin with a capital letter
11 | ERROR | Class name must use UpperCamel naming without underscores
33 | ERROR | Public method name
| | "views_handler_field_user_revision_edit_link::render_link" is not
| | in lowerCamel format, it must not contain underscores
--------------------------------------------------------------------------------

I see this issue was addressed in the previous comments (#11), and i agree on that.

Manual Review

In general everything looks pretty good. Some issues I've found are:

1. hook_views_api: The last stable release is 3.8. Instead of defining a specific release (3.0-alpha1) better to put just '3'
2. user_revision_edit.module line 128: don't understand $revision->revision = 1. It is not a property, and not saved to the database. Think this can be removed.
3. user_revision_edit_form_validate does quite a few things for validation that the original 'user_account_form_validate' does do too. If possible I would add the 'user_account_form_validate' as a validation callback so you can save yourself some coding. Maybe you have considered this option, and have reasons not to use the core user_account_form_validate?
4. user_revision_edit.admin.inc line 244: is it necessary to 'cache_clear_all();' ? On bigger websites this will cause considerate waiting times upon submit.
5. In hook_menu_alter, line 51 user_revision_edit, and also other places I would prefer to have the definitions on one line without a hard return. I know that the line would exceed the threshold of 80 characters, but I think it's better for readability and Drupal Coder won't throw an error. This is my personal preference though.

I'm not changing the status to 'needs work' since these issues are not really show stoppers I think, but will leave the decision to RTBC to someone else...

Automated Review

PAReview came up clean.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes

Is adequately addressed on the project page. Quote:

My module is not a duplicate. It's extending the functionality of the User Revision module. In addition, this should not added to the User Revision module because modifying revision is not the default desired behaviour.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements

3rd party code/content

Yes: Follows the guidelines for 3rd party code.

README.txt

No.
The documentation is too brief. For instance, the module creates two permissions. Configuration of permissions and their security implications is not documented. Please see the guidelines for in-project documentation. Not a blocking issue, tho'

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes: Follows guidelines for writing secure code.

Coding style & Drupal API usage

Code walkthrough did not reveal anything wrong with the code submitted.

There are IMHO no more blockers. Moving to RTBC. Note that promotion will not happen until a git administrator has given this a second set of eyeballs.

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

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

  FILE: /home/klausi/pareview_temp/user_revision_edit.module
  --------------------------------------------------------------------------------
  FOUND 2 ERRORS AFFECTING 2 LINES
  --------------------------------------------------------------------------------
   124 | ERROR | [ ] Inline comments must end in full-stops, exclamation marks,
       |       |     or question marks
   125 | ERROR | [x] A cast statement must be followed by a single space
  --------------------------------------------------------------------------------
  PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
  --------------------------------------------------------------------------------
  

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. user_revision_edit_link_access(): $permissions is unused? Just remove it and from hook_menu()?
2. user_revision_edit_link_access(): Do not use db_select() for simple static queries, use db_query() instead. See https://www.drupal.org/node/310075
3. user_revision_edit_save(): @param docs missing.
4. user_revision_edit_form(): why do you need drupal_retrieve_form() and drupal_prepare_form()? Shouldn't you simply use drupal_get_form()? Please add a comment.
5. user_revision_edit_form_submit(): why do you need to clear ALL the chaches here? Please add a comment.

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

Thanks for your contribution, donSchoe!

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.