[D7] Only One

Fixed the git clone URL in the issue summary for non-maintainer users.

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.

Automated Review:

Review of the 7.x-1.x branch (commit 6da040e):

Coder Sniffer has found some issues with your code (please check the Drupal coding standards).
FILE: /root/repos/pareviewsh/pareview_temp/README.txt
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
28 | WARNING | Line exceeds 80 characters; contains 122 characters
----------------------------------------------------------------------

Time: 99ms; Memory: 8Mb

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.

Manual Review:

Individual user account
Yes: Follows the guidelines for individual user accounts.

No duplication
Yes: Does not cause module duplication and/or fragmentation.

Master Branch
Yes: Follows the guidelines for master branch.

Licensing
Yes: Follows the licensing requirements.

3rd party assets/code
Yes: Follows the guidelines for 3rd party assets/code.

README.txt
Yes: Follows the guidelines for in-project documentation and/or the README Template.

Code long/complex enough for review
Yes: Follows the guidelines for project length and complexity.

Secure code
Yes: Meets the security requirements.

Coding style & Drupal API usage
List of identified issues in no particular order. Use (*) and (+) to indicate an issue importance. Replace the text below by the issues themselves:
It is good to put drupal template file in template directory.
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.

Personal Feedback:
Seen one of the best and complete code writing for a module with all the comments and complete readme file. Nice work :)

adriancid you can use various shorl url services like google url shortner or bit.ly to make your url short and use that short url in README. Other codes seems fine.

Thanks

Codesniffer is a tool that recommends best practices. Using a url shortner to pass code validation for a URL in a readme file is nonsense. However, if you must, just use the nid (www.drupal.org/node/895232) ;-)

Overall the code looks good. I have some small things regarding the Drush part:

• Do you really need 4 separate commands to show configured, available and non-available content types and a combination of all? IMHO only the summary would suffice.
• onlyone.drush.inc:104: $index = array_search(0, $onlyone_content_types, 1);

  The third parameter is supposed to be a boolean value, so it would be better to use TRUE instead. Since you validate the inputs against known content types, I doubt if it ever would be possible to get a 0 there, but then again it is probably there for a reason.

• There are some very very small spelling issues: 'The following content types are not availables to have Only One node: @names' and more alike. But those should not block your project application.

The code is very well written and documented and I could not find any potential issues.

Please see my updated post for some remarks.

Yes my english is not the best, if you can help me with this, this will be great ;-)

It is irrelevant for your project application, but I'll open an issue in your sandbox issue queue somewhere this weekend.

Adrian, can you merge your commands to reduce the number? You can create Drush commands that uses arguments. For example, see drush help role-list.

Merge:

1. drush onlyone-list
2. drush onlyone-available
3. drush onlyone-not-available

These can stay as is:

1. drush onlyone-summary
2. drush onlyone-enable
3. drush onlyone-disable

I did not test the Drush commands yet. However, I see a small UX problem that should be fixed:

The Add Content page still list the type of content that have been configured to accept a Only one node.

When a node Add is disabled, you should at minimum highlight which content type won't accept new content (in fact providing node edition instead).

Can you change the node/add form to something like:

<ul>
  <li><a href="/node/add/page">Edit</a> Basic page (Only one Basic page can be created and edited)</li>
</ul>

Or simply to:

<ul>
  <li><a href="/node/add/page">Edit</a> Basic page</li>
</ul>

Yes! I think we can safely mark this issue as fixed as soon as you create the new feature request in your project page.

You will get more exposure and more people will look at your issue queue.

Git clone failed for https://git.drupal.org/sandbox/adriancid/2674476.git while invoking http://pareview.sh/pareview/httpsgitdrupalorgsandboxadriancid2674476git

Git clone failed. Aborting.

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

Looks good to me.

Small nitpicking may be to make the module title a bit self-explanatory. Machine name is perfectly OK, but the user-facing title could have more information what this module is about.

In the hook_form_alter implementation, I noticed that it is executed on forms where it should not have. hook_form_alter is invoked for all the forms, but we only need to alter the node form at this point. If you rename onlyone_form_alter function to onlyone_form_node_form_alter (implementing hook_form_BASE_FORM_ID_alter), that function is guaranteed to be called only node edit/add forms. You could also access the full node object and its type (even on node-add forms) and save yourself from substr hassle.

I did test it and worked really well. Kudos on great work nicer phpdoc, comments, and clever SQL! If there are no objections, I will mark this fixed in a few days.

My 2 cents about #18 ...

To be honest when I first heard about your module, I also wondered something like "What is it supposed to do" (I needed to read a bit to find the answer to my question).

Example: if I use (for some other module I co-maintain) "Forena" or "Forena Reports", clearly the last indicates it has to do with Reports, so you may make me curious. So how about something like "Allow a content type only once"? Or variation (better fit with machine name): "Only allow a content type once"? Imagine you get 5 secs on a TV show to promote you module, then "Only allow a content type once" would be a possibly solution to "Deliver your, terribly expensive, message (to create a first impression)". Makes sense, a bit?

PS: if you ever want to consider improving your module from "node" to "entity" you could probably adapt your module's title accordingly ...

Pierre's suggestions are great, and I would suggest renaming the module title to one of his example too. Seeing as there are no objects regarding code, I'll go ahead and complete this.

Thanks for your contribution, Adrian !

I updated your account so you can opt into security advisory coverage now.

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.

Assigning credits.

DrupalPractice has found some issues with your code, but could be false positives.
FILE: /root/repos/pareviewsh/pareview_temp/onlyone.module
--------------------------------------------------------------------------
FOUND 0 ERRORS AND 5 WARNINGS AFFECTING 5 LINES
--------------------------------------------------------------------------
13 | WARNING | Global constants should not be used, move it to a class
| | or interface
14 | WARNING | Global constants should not be used, move it to a class
| | or interface
16 | WARNING | Global constants should not be used, move it to a class
| | or interface
17 | WARNING | Global constants should not be used, move it to a class
| | or interface
19 | WARNING | Global constants should not be used, move it to a class
| | or interface
--------------------------------------------------------------------------

.Codespell has found some spelling errors in your code.

./onlyone.drush.inc:214: cant ==> can't
./onlyone.drush.inc:215: cant ==> can't
./onlyone.drush.inc:228: cant ==> can't
./onlyone.drush.inc:229: cant ==> can't
./onlyone.drush.inc:270: cant ==> can't
./onlyone.drush.inc:271: cant ==> can't
./onlyone.drush.inc:284: cant ==> can't
./onlyone.drush.inc:285: cant ==> can't
./onlyone.module:182: conection ==> connection
./onlyone.module:259: cant ==> can't
./onlyone.module:260: cant ==> can't