Module description

This module is useful for sites that share a guest account by more than one person. You may want to consider creating a role "guest" and one user "Guest user" that only belongs to that role. Like the Anonymous user, the Guest user should not be able to edit anything on the site, but just have additional view (download) access. This module adds a permission called "edit own user profile". If you ensure the roles "anonymous user", "authenticated user" and "guest" all do NOT have granted that permission, while all other roles do have it granted, the Guest won't be able to change anything on her profile, while all other authenticated users will still be able to do so.

Please see the project page for more information, including a comparison with similar modules and its difference.

Project page (Sandbox)

Git repository

  • Drupal 6.x: git clone git://git.drupal.org/sandbox/roball/1242232.git -b 6.x-1.x guest
  • Drupal 7.x: Not yet available

Comments

klausi’s picture

Status: Needs review » Needs work
  • Please remove the LICENSE.txt file. Drupal will add the appropriate version automatically during packaging so your repository should not include it.
  • ReadMe.txt should be README.txt, see also http://drupal.org/node/447604
  • "! user_access($module_constants['PERMISSION_EDIT']": there should be no space after "!". Also elsewhere. Run coder to check your code style: http://drupal.org/project/coder
  • hook_help(): text in there should run through t() for translation. Do not pass variables to t() as translation tools will not find the text then. See http://api.drupal.org/api/drupal/includes--common.inc/function/t/6
  • "# Item 1:": do not use "#" style comments, use "//". See http://drupal.org/node/1354#inline
  • _guest_get_module_constants(): is this function really needed? Just define your constants in the global space of your module (prefixed with your module name of course).
  • DRUPAL_URL: why do you need that? the l() function works with relative paths, too.
glekli’s picture

I did a brief review on your module. Here are my thoughts:

- ReadMe.txt should be README.txt.
- There is one line in it that is longer than 80 characters.
- I don't think you should include screenshots or .htaccess in the module.
- Is there a need to hardcode the url to the project page? The Available updates menu will link to the project page.
- It looks strange to me that you redefine your constants in every function by calling _guest_get_module_constants(). Can you define them as real constants by using define(), and make sure that you only define the ones that are really needed? I don't think it is necessary to define the module path as a constant, for instance.
- I don't think it is necessary to define the module name as a constant either. In module_load_include, for example, you can just type the module name and the include file. The module name won't change very often, and if it does, you have to go through the whole module and replace the name everywhere anyway.

glekli’s picture

I'm sorry, I did not realize you posted a review while I was editing mine.

roball’s picture

Thank you both klausi and Gergely Lekli for your reviews.

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

Done.

ReadMe.txt should be README.txt

Done.

There is one line in it that is longer than 80 characters.

Cleaned up.

"! user_access($module_constants['PERMISSION_EDIT']": there should be no space after "!". Also elsewhere. Run coder to check your code style

Done. I have already had run my code through Coder 6.x-2.0-rc1 which did not complain, even in "minor (most)" mode.

do not use "#" style comments, use "//".

Done.

hook_help(): text in there should run through t() for translation. Do not pass variables to t() as translation tools will not find the text then.

This is already done this way, except in one case where no translation is explicitly desired, where I just print out the entire content of the README.txt file through Drupal's integrated help menu. In that (one) case I am using check_plain() instead of t() to ensure security sanitation is still applied.

_guest_get_module_constants(): is this function really needed? Just define your constants in the global space of your module (prefixed with your module name of course).

The function is not essentially needed, but personally I prefer that way over defining global constants which would pollute the $GLOBALS array throughout all used modules.

UPDATE (see below): Removed that function.

DRUPAL_URL: why do you need that? the l() function works with relative paths, too.

I am using such a constant because within my module's help section (at admin/help/guest) there are some links that would not work properly when Clean URLs would be turned off. This way the links work properly regardless of the Clean URLs setting.

coderintherye’s picture

I'm going to agree with the other two posters that your _guest_get_module_constants() function is awkward, and does not conform to the standard of prefacing your constant names with the module name. Please try to stick to established standards as it makes code a lot easier to maintain and read. On that note typically authoring information is included in a docblock section at the top of the module file and/or in the readme as opposed to in defined Constants, though that is more just of a recommendation.

roball’s picture

klausi’s picture

Status: Needs review » Reviewed & tested by the community

guest_help(): the problem with passing variables to t() is that source code scanning tools will not pick up the text. So it is possible to translate the text in the Drupal translation interface, but it will not show up on localize.drupal.org for example.

Anyway, this is a rather minor detail. Otherwise RTBC for me.

roball’s picture

Thanks a lot klausi for your detailed evaluation of my and lots of other submissions!

I have now replaced the variable passed through t() by its actual string, so I think all requests are done.

attiks’s picture

Status: Reviewed & tested by the community » Needs work

Some minor questions:

  1. why is there an .htaccess file
  2. there's a license inside the README.txt
  3. GUEST_DRUPAL_URL inside .install and includes/guest.help.inc
  4. Example img contains upper case and spaces: 'Permission edit own user profile.png'
  5. $link_permissions_img (inside includes/guest.help.inc), why not use theme_image (http://api.drupal.org/api/drupal/includes--theme.inc/function/theme_image/6)
  6. why no direct link to the readme, why the need to use hook_menu?
  7. .module:
    • define('GUEST_MY_PATH', dirname(__FILE__) . '/'); => drupal_get_path('module', $module)
    • define('GUEST_DRUPAL_URL', ...), if you use l/url functions there's no need for this
    • GUEST_DEBUG === TRUE, but GUEST_DEBUG isn't defined
roball’s picture

Status: Needs work » Needs review

Thank you attiks for your tips.

there's a license inside the README.txt
GUEST_DRUPAL_URL inside .install and includes/guest.help.inc
Example img contains upper case and spaces: 'Permission edit own user profile.png'
$link_permissions_img (inside includes/guest.help.inc), why not use theme_image (http://api.drupal.org/api/drupal/includes--theme.inc/function/theme_image/6)
.module:
define('GUEST_DRUPAL_URL', ...), if you use l/url functions there's no need for this
GUEST_DEBUG === TRUE, but GUEST_DEBUG isn't defined

All those suggestions are now committed.

why is there an .htaccess file

To add a piece of security. I don't want the included .txt files to be readable by everyone, by simply accessing an like URL http://example.com/sites/all/modules/guest/README.txt. Should there ever be a known vulnerable version hackers could easily spy out affected sites. Sure this won't work with all webservers, but it should work with most Apache installations at least.

why no direct link to the readme, why the need to use hook_menu?

See above.

define('GUEST_MY_PATH', dirname(__FILE__) . '/'); => drupal_get_path('module', $module)

I have that constant defined as GUEST_MY_URL.

attiks’s picture

Status: Needs review » Reviewed & tested by the community

I don't see how the .htaccess file is adding any protection, this is the first time I see this kind of protection, so rather remove it and link to the readme file directly and you can remove GUEST_MY_PATH as well.

Since this is open for debate, someone else has to give feedback on this as well.

Temporarily marking this RTBC, so we can solve this asap.

klausi’s picture

Status: Reviewed & tested by the community » Fixed

I'm not aware of a policy regarding .htaccess files in contributed modules, so I would leave that to the module author. Don't think we should hold this back for just that reason, so ...

Thanks for your contribution, roball! Welcome to the community of project contributors on drupal.org.

I've granted you the git vetted user role which will let you promote this to a full project and also create new projects as either sandbox or "full" projects depending on which you feel is best.

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.

As you continue to work on your module, keep in mind: Commit messages - providing history and credit and Release naming conventions.

roball’s picture

Thank you for the approval! Glad to be an official member of Drupal's contribution community.

Maybe this is the wrong place to mention the following problem: After promoting the project from a sandbox to a full project, the sandbox URL (in my case http://drupal.org/sandbox/roball/1242232) no longer exists. It would be useful to redirect it to the new URL (in my case http://drupal.org/project/guest), instead of returning an Error 404.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.