[D7] hamdomains

Multiple Applications

It appears that there have been multiple project applications opened under your username:

Project 1: https://drupal.org/node/2143623

Project 2: https://drupal.org/node/2184079

As successful completion of the project application process results in the applicant being granted the 'Create Full Projects' permission, there is no need to take multiple applications through the process. Once the first application has been successfully approved, then the applicant can promote other projects without review. Because of this, posting multiple applications is not necessary, and results in additional workload for reviewers ... which in turn results in longer wait times for everyone in the queue. With this in mind, your secondary applications have been marked as 'closed(duplicate)', with only one application left open (chosen at random).

If you prefer that we proceed through this review process with a different application than the one which was left open, then feel free to close the 'open' application as a duplicate, and re-open one of the project applications which had been closed.

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

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

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.

Your sandbox repository seems to be empty. See https://drupal.org/project/2137861/git-instructions.

I see that your code is attached as zip files on the sandbox project page. Drupal.org instead uses Git to manage all project code. If you are unfamiliar with version control using Git there is an excellent free ebook at http://git-scm.com/book.

Once you have created a git repository and committed your code you can upload (push) it to your sandbox by following the instructions under "Version Control" on your project page.

Git clone command for the sandbox is missing in the issue summary, please add it.

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

Git clone command for the sandbox is still missing in the issue summary, please add it. Please follow https://drupal.org/node/1011698 to fill out your issue summary.

• The .info file for this module contains a character code at the beginning that doesn't render in all text editors (it doesn't display in sublime text but I was able to see it as an empty space in nano) but because it's read as being before the name value in the file it causes the Drupal core system for displaying and sorting modules to throw a repeated error: Notice: Undefined index: name in system_sort_modules_by_info_name().
• Initial comment block in the .install file still references the example module that the code was cloned from: Install, update and uninstall functions for the block_example module.
• Many coding style issues, please take a look at the Drupal coding standards for guidelines on what needs to be adjusted: https://drupal.org/coding-standards
• Mix of tabs and spaces used for code indenting (see coding standards for guidelines).
• hamdomains_help creates hard-coded links as markup that might be better created through the use of the l() function (to provide i18n/l10n hooks): https://api.drupal.org/api/drupal/includes%21common.inc/function/l/7
• hamdomains_permission() - the permission that is created "view links" seems ambiguously named and seems likely to have issues with other (commonly used) existing modules (such as views and link), maybe try prefixing this using the module name.
• In cases where database queries are included as a whole in the codebase (for example in hamdomain.module as part of hamdomains_block_content() on line 348) please consider rewriting that code to use the DB API to help insure the queries will work when used with different DB server engines. See: https://drupal.org/developing/api/database
• The theming approach used in hamdomains.tpl.php injects a LOT more logic into a TPL file that should probably be in there. TPL files are typically mostly HTML with enough PHP to insert values, this file is mostly PHP with a bit of HTML mixed in.

Based on the project description where you mention "...Please consider that I hold its copyright", I'm sorry but it seems if you are not willing to release the code under GPL, so Drupal.org admins won't approve this application. I see that you have put a lot of effort to this (as you have mentioned a freelancing developer as well), but all code hosted on drupal.org or drupalcode.org requires to be in GPL (you'll probably need to discuss with this with the developer also).

Some improvements/changes:

.install file

- No need of hamdomains_uninstall hook implementation here. It's trying to drop the database defined in hook_schema, which, since Drupal 7 does automatically.
- I'd not store a background color or anything related to styling that can be changes easily with CSS. Feel free to put CSS classes, even with a unique identifiers for the block, so module users can use CSS to style it as they exactly need.

.module file

As above comment mentioned, hamdomains_permission uses a common permission name which might conflict with other modules. In the front end, site admins will see permissions grouped by the module name. But in the code, there is no module-based separation in the permission name.

If the validation functions are not widely used, consider putting your form validation logic in the form's validation function. element_validate is generally for basic validations which can be used almost everywhere.

In some database calls, why uid is always 1 ? If not necessary, I'd remove that column from the table entirely.

There are some insecure db_query function calls (line 287 in 1a23a695aeeab9c58dd602c29d25bf47d995489e commit for example)

Also, I think the current approach of allowing other users to access your MySQL database directly is a huge security issue for your own site. I'd develop some sort of API and access that as opposed to giving out database a username/password and even encouraging them to access the database directly.

Closing due to lack of activity. Feel free to reopen if you are still working on this application (see also the project application workflow).

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

There is an error popping up on the hamdomains.module file on line 360. The variable '$fields' is an array. But you can see on line 355 that the variable hint is included in the &params comment line.

The variable hint goes in the function declaration, not in the docblock.
What you have in the docblock is documentation, not a PHP variable hint. Try:

function hamdomains_get_default_value(array $field = NULL, $uid = NULL) {

 

I think the following statement at the end of README.txt is confusing;

I submit this module as Open Source but I retain all rights to this software.
Please consider that I hold its copyright.

You don't retain all rights (as in "all rights reserved") when you release software under GPL V2. You retains its copyright, and all the rights that isn't already licensed under GPL V2.

I suggest you replace those two sentences with the following:

Copyright © 2014 Fred Atkinson

The automated tests are referring to this

function hamdomains_get_default_value($field = NULL, $uid = NULL) {

it should be

function hamdomains_get_default_value(array $field = NULL, $uid = NULL) {

In a few minutes, I'm going to make new .zip and .tar.gz files to put on the Sandbox page.

There is no harm in that, but IMHO, it is waste of time. I can't imagine anyone here using anything but git to interact with your project repository.

If I put 'array' in front of $fields on that line, the module crashes when I try to run it.
If you have any other suggestions, I'll be glad to hear them.

Well, in front of the parameter is where the type hint should go. See http://php.net/manual/en/language.oop5.typehinting.php

However NULL doesn't look like a valid array. Try:

function hamdomains_get_default_value(array $field = array(), $uid = NULL) {

@gisle Good catch, I totally missed the $field = null part.

Try pushing again. Sometimes git throws that error.

Hey @fatkinson. What does running this command, return:
git remote -v ?

You can't push to the public repo - you need to add your personal as a remote. Do this:
git remote add personal fatkinson@git.drupal.org:sandbox/fatkinson/2137861.git
Then, add and commit files normally. When you are ready to push, run:
git push -u personal 7.x-1.x

Hello fatkinson,

The error is: 378 | ERROR | Type hint "array" missing for $field :
function hamdomains_get_default_value(array $field = NULL, $uid = NULL)

Ref : PHP.net

Automated Review

FILE: /var/www/drupal-7-pareview/pareview_temp/hamdomains.module
--------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------------
378 | ERROR | Type hint "array" missing for $field
--------------------------------------------------------------------------------

FILE: /var/www/drupal-7-pareview/pareview_temp/README.txt
--------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------------
113 | ERROR | Files must end in a single new line character
--------------------------------------------------------------------------------

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

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

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes.

Coding style & Drupal API usage

There're really too many conceptual module mistakes that needs to be fixed in order to make this module contributed.

1. (*) First off all, there's no need of using separate table to save singular configuration data of your module. Meaning no need of '.install' file and hamdomains_get_default_value function. All you need is Drupal's core functions variable_set and variable_get
2. (*) Secondly that's a really bad practice to make direct requests to external DB (I'm talking about the DB at 'wb4aej.info' host). The best practice is to provide additional layer, which will grab data from your DB at 'wb4aej.info' server and output it as text/json/xml/whatever, meaning create nice service (interface) which could be used not only by tools which can connect to DB. Anyway, if you still want to stick with using DB requests directly, then inside you module you need at least use Drupal API for DB connection/queries (see: https://www.drupal.org/node/18429) instead of mysql_connect, mysql_query etc. If you do not use Drupal API, then there's no point of writing modules for Drupal.
3. (*) I did not get where do you get from 'view links' permission and what for hamdomains_access function. I guess you need to use already defined permission through hook_permissions 'hamdomains' and user_access function
4. (*) Inappropriate output of HTML code inside your template. Currently you are doing echo "some html code";. This way Drupal TPL files are loosing their sense. They are TPLs because you need to write HTML code directly without any print, echo. Using print is permitted only if you want to output some PHP value, e.g. <div class="test"><span><?php print $column?></span></div>

That's the worst things I've found. There're more, but first lets fix those above, otherwise I'm afraid module will never get approved.

Based on the review of @nostop8 this module needs more work.

Also noticed there isn't activity lately in this issue.
If you are still working on this module please do the changes, otherwise if you have abandoned this module please close this issue to help the webmasters work.
https://www.drupal.org/node/894256#unsupported-module

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

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