[D7] Node Author

A couple of notes before I even get into a closer review:

- your .info file is not complete. It only has a single line for the name. You should at a minimum add a description field and specify which version of core the module is compatible with.

- PAreview gives a ton of formatting errors. They seem all very easy to fix and you should read through them to make sure you comply with the coding standards.

- you should not use generic names like "callback_checkbox_field" as function names. Choose something that is less likely to be in conflict with other modules, for example "_node_author_callback_checkbox_field".

- I am pretty sure the permissions check will not work as it is implemented now: The permission is set as "Administer node author", the check is for "administer node author". Remember string comparisons are case sensitive in PHP.

More later once I have a chance to go through more details...

Automated Review

http://pareview.sh/pareview/httpgitdrupalorgsandboxkaran2246897git-7x-1x (commit 28bd6d3) found a long list of problems. Recommend taking a close look.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements

3rd party code

Yes: Follows the guidelines for 3rd party code.

README.txt/README.md

No: There was neither a README file nor a project page to review.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity. NOTE: Although the code is not overly complex, it does allow a sufficient review.

Secure code

Yes. If "no", list security issues identified.

Coding style & Drupal API usage

Setting status to "Needs work" for a number of reasons: issues found below, lack of documentation, long list of coding standard violations. Also, depending on documentation, there might be ways to achieve the same functionality with existing modules. Documentation should explain what is different about this approach.

1. (+) Module sets a variable "profile-selected" but does not remove it upon uninstall
2. (+) Main admin can edit the module configuration, but permissions can not be assigned due to error in permission string.

@Karan: PAreview does indeed come back clean now. However, take a look at the note about documentation. I don't think we'd want to see a module released without ANY documentation whatsoever. I also added a slight concern about code duplication in the section about code review. I'm hoping that it will be implicitly addressed in the README file. If not, please explain how you would solve the same problem with existing code and what your module does to improve the situation. Kept the status as "needs work" for that reason.

Take a look at: https://www.drupal.org/node/1011698 for information about the correct process for the Project Application.

Spefically Step 5, you are missing significant information needed for a proper review to be done.

• Title should contain Project Name and should indicate which version of drupal the project uses. Take a look at others submitted
• issue should contain detailed description of what your project does, and how it differs from other similar projects.
• A git clone command, this saves reviewers from having to dig around to find the clone command.

project page

Please take a moment to make your project page follow tips for a great project page.

License

Please remove the LICENSE.txt file. Drupal will add the appropriate version automatically during packaging so your repository should not include it.

Automated Review

Best practice issues identified by pareview.sh

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Unsure: Does not cause module duplication and fragmentation.

Not enough information provided on the project page or readme file to judge what exactly the poject is for.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements

3rd party code

Yes: Follows the guidelines for 3rd party code.

README.txt/README.md

No: Follows the guidelines for in-project documentation and the README Template.

Readme file is lacking enough information

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Unsure - errors prevent full testing. If "no", list security issues identified.

Coding style & Drupal API usage

1. (*) Major finding
2. Minor finding
3. (+) Release blocker

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

This review uses the Project Application Review Template.

    Notice: Trying to get property of non-object in node_author_block_view() (line 219 of /var/www/test-drupal/sites/all/modules/node_author/node_author.module).
    PDOException: SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'AND `type` = 'main'' at line 1: SELECT `pid` FROM `profile` WHERE `uid` = AND `type` = 'main'; Array ( ) in node_author_block_view() (line 244 of /var/www/test-drupal/sites/all/modules/node_author/node_author.module).

You need to have three full reviews of different projects to qualify for the review bonus. The first two are duplicates of the same project and all three are just single comments rather than full reviews. Removing review bonus tag for now.

Reading through the latest information in the description above, it seems to me that this would normally be done with the help of a view. Is this correct? If so, how is this approach different and does it justify a standalone module? (not saying it doesn't - just asking for clarification).

Also, notice this module: https://www.drupal.org/project/node_author. It seems to do the exact same thing for Drupal 6.

So I suspect the decision to be made is whether to fold this into the existing module as the v7 branch, or whether the Drupal 7 views might already cover the same functionality thus making a version for D7 unnecessary.

I have to say I still don't understand why this would not be a 7.x branch of the existing module. Changing dependencies are nothing unusual - they exist in other modules as well. And if nothing else, I think we should try to avoid the confusion that comes from having two modules called "Node Author" that are both intended to solve the same problem, just for different versions of Drupal. I don't claim to have the ultimate insight on this topic, but it does seem a little strange. Have you contacted the author of the other module?

Marking this as "Closed (won't fix)" for the reasons given above. Most importantly, the duplication concern is not addressed. There is an existing module with the identical functionality for Drupal 6 and having a separate module for Drupal 7 makes very little sense, IMHO. Recommended course of action: Contact the maintainer(s) of the existing module to contribute to the Drupal 7 version.